[WIP] Add diagnostics to batching

[SeanFeldman]

Adding diagnostics to batching branch to see how this would work.

[SeanFeldman]

@lmolkova I need your help 😊

batching is my attempt to implement safe batching (size safe batching allowing to add messages that are within max message size). More details here.

The issue I'm running into is how ServiceBusDiagnosticSource is implemented. It always expects to have access to a list of messages that are sent. With safe batching, to avoid double-serialization, that list is not available. I.e. messages cannot be modified once they are added to a batch. Is there a way to participate in diagnostics w/o operating on all messages? Are there any other options? Thanks.

[lmolkova]

@SeanFeldman diagnosticsource stamps ids for correlaiton on all messages, so that any consumer that will receive any message can correlate telemtery to the producer. So it has to operate on all messages and the same messages should be sent i.e. it should not modify the copy which never leaves process.
Does this change modifies the messages that will be sent?

There is another option we are considering to better support batching scenarios, which is to stamp the message when it's created, not when it is sent. We've been planning to work on this in late Ti, but have not finalized dates yet.

Could we follow this approach:

1. ensure all messages are stamped (on send) now, it could be somewhat not optimal, but should be skipped if if diagnostics is disabled
2. me (or someone else from my team) will work on better support for batching and we'll optimize this case.

[SeanFeldman]

Does this change modifies the messages that will be sent?

Not quite. The problem at hand is different though. When batch is constructed, we don't know what messages are going to make it into batch and which will not. Messages that make it are converted into AMQP messages and cannot be modified anymore. Here's a pseudo code to elaborate.

var batch = messageSender.CreateBatch();
// message added as batch size is less than allowed maximum batch size; returns true
batch.TryAdd(message1); 
// message added; returns true
batch.TryAdd(message2);
// message not added as it would cause batch to exceed max size; returns false
batch.TryAdd(message3);

To determine if a batch has reached its maximum or not, Service Bus Message has to be converted to AMQP data. Once that happens, it cannot be modified. Therefore each added message can potentially reference message IDs of the previously added messages, but not those that will be added later. I.e. message2 can have a reference to message1.Id, but message1 will know nothing about message2.Id.

behind the scenes what happens is:

1. tryadd(message1) -> message1 converted to amqp1data,total+amqp1 < max, total = amqp1` and stored in batch
2. tryadd(message2) -> message2 converted to amqp2 data, total+amqp2 < max, total += amqp2 and stored in batch
3. tryadd(message3) -> message3 converted to amqp3 data, total+amqp3 => max not stored

therefore once a ServiceBus Message is converted, it cannot be modified again.

There is another option we are considering to better support batching scenarios, which is to stamp the message when it's created, not when it is sent.

Sounds like that approach would work as it won't require to have all messages in order to stamp those as a group. That is assuming the scenario described above would work.

[lmolkova]

@SeanFeldman so batching is a NEW API, right? Does anything changes for users which use current Send(message) or Send(List<Message>)?

If it's a new API, I think it's reasonable to have this API first without automagical propagation. I'll discuss it offline with the team and we'll try to prioritize work on batching diagnostics support. We'll also try to provide some guidance on steps users should take to enable correlation manually.

[SeanFeldman]

@lmolkova correct. This is a brand new API proposed here. Send(Message) and Send(List<Messages>) are not affected.

If it's a new API, I think it's reasonable to have this API first without automagical propagation.

👍

I'll discuss it offline with the team and we'll try to prioritize work on batching diagnostics support. We'll also try to provide some guidance on steps users should take to enable correlation manually.

That would be fantastic.

[nemakam]

There is another option we are considering to better support batching scenarios, which is to stamp the message when it's created, not when it is sent.

I guess this is a good approach.
@lmolkova Once we stamp a message with an activity, and start it, if TryAdd() returns false, i.e., it could not be added to the batch, then is there a way to cancel that activity and produce that as a no-op so that it doesn't come up as failure?

[lmolkova]

@nemakam good question. It seems that adding a message is not a real activity - it does not have a duration and result. What we actually need is to assign a unique Id that will be propagated along with the message and make sure it has proper tracing context (for correlation with other related telemetry). Perhaps if TryAdd to back was not successful, we should not even generate such id and stamp it on the message.

We'll need to figure it out once we start working on the batching.

[nemakam]

Perhaps if TryAdd to back was not successful, we should not even generate such id and stamp it on the message.

TryAdd makes the message immutable. Once it is invoked, messages are not supposed to be updated. So stamping "later" is not quite feasible.

We'll need to figure it out once we start working on the batching.

Sure. Just something to keep in mind.