[Event Hubs] Incorporate AmqpAnnotatedMessage into EventData

[jsquire]

Summary

The focus of these changes is to introduce the AmqpAnnotatedMessage abstraction as the basis for EventData, allowing access to the full set of AMQP message data. This enables scenarios such as interaction with other message brokers that make use of the protocol in ways that are not otherwise exposed within the Event Hubs client.

This set of changes will be followed by refactoring of the AmqpMessageConverter to work with the full AMQP message rather only than the known EventData subset.

References and Related

• Event Hubs: Extend EventData with AMQP Message Details (#20105)

[jsquire]

/azp run net - eventhub - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[christothes]

Is the default a value one would ever use, or is it intended as a sentinel value for "this failed to return a useful value"?

[christothes]

same question for the others.

[jsquire]

This is the common default for EventData to keep parity with the existing value. I could have removed the default and force callers to provide one, since EventData is doing so explicitly, but this let me be a bit lazier in the tests and take advantage of existing code that assumed the event data defaults.

[christothes]

Is perf a concern at all for this type?

[jsquire]

Yes and no; the callers for SystemProperties aren't many and won't be calling in a tight loop. That said, this isn't optimal with respect to performance for callers who are probing the dictionary frequently. Unfortunately, with AmqpAnnotatedMessage being fully mutable and offering nothing like INotifyPropertyChanged to detect updates, we can't safely cache values here or even generate an actual dictionary with copied data.

If you've got an idea, I'm all ears. This was the best trade-off that I could come up with for safety and though I don't particularly like the performance implications, I believe they're reasonable for this context.

[christothes]

Not sure if this is better without profiling, but I was thinking something like a Dictionary<string, Func<AmqpAnnotatedMessage, object>>. Then an example get below would look like:

return _dict[key](_amqpMessage);

[jsquire]

The SystemProperties dictionary on EventData is part of the GA surface, so it has to remain IReadOnlyDictionary<string, object> with respect to the public interface. In theory, we could wrap your proposed implementation.

That said, there's a couple of considerations that I see:

• Because the data comes from mutable sources in the underlying AMQP message, we don't have a stable set of keys to pre-populate in the key-to-func dictionary. The well-known properties would work as accessors, but we'd need to do the same condition check to determine a response for ContainsKey since we don't want the key to be present if the section or value is not populated. (This is to preserve existing behavior, there are no null values ever added to the system properties set in GA).

• Since we can't capture the set of annotations and create keys for them in the func dictionary, because items may be added/removed from the underlying set, we'd have to find a way to intercept any unrecognized key and dynamically map to the annotations section.

• Because each section of the AMQP message does lazy allocation when you use the accessor, we want to continue to guard with HasSection before accessing the property directly, so that we don't trigger unnecessary allocations. We could do this in the func, but it prevents a simple pass-through.

• I see us having to replicate the majority of logic from the custom dictionary projection, since we can't assume a stable set of keys to prepopulate the func dictionary and we can't cache; I worry that we'd end up allocating a set of func instances that just delegated to something similar to this form. I don't see us saving much in performance since we'd have to run much of the same logic, and I see us eating more allocations in that scenario.

It could be that I'm overlooking some of your thoughts. If you're game, I'd like to move forward with the current form to not lose momentum and continue talking over your ideas to potentially follow in another set of changes.

[christothes]

Could this be simplified to?:

Suggested change

	if (_amqpMessage.HasSection(AmqpMessageSection.Properties))
	{
	return _amqpMessage.Properties.MessageId?.ToString();
	}
	return null;
	return _amqpMessage.Properties.MessageId?.ToString();

[jsquire]

The message sections are lazily allocated when the accessor is called. If we were to go with the pattern that you're suggesting, it would work functionally, but would force an unwanted allocation in the event that the properties section was not created/populated.