EventHub EventSource remove allocations and boxing

[danielmarbach]

This PR adds two additional overloads of WriteEvent that take four or five strings to make sure the param overload is not used and hence doesn't heap allocate anymore.

Very similar to for example

https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/Common/src/System/Net/Logging/NetEventSource.Common.cs#L380

I'm not using a SetEvent method since I felt this might be overkill here

This requires enabling unsafe on the project. If you are OK with this approach, I will finish the few remaining methods that also box some value types.

FYI there are over 60 usages of WriteEvent that use the param based overload and/or box value types.

[ghost]

Thank you for your contribution @danielmarbach! We will review the pull request and get back to you soon.

[danielmarbach]

@jsquire what do you think about this one?

[jsquire]

Interesting. If I'm following, we're overloading to explicitly handle cases that would normally trigger this overload to avoid the object[] allocation. I don't think that I ever really dug into EventSource at all and realized we'd hit a params array after 3.

Assuming that I've got that correct, I'm good with the approach. We've got other libraries, like Azure.Core allowing unsafe blocks, so there's precedent. I know that Event Hubs, specifically, commonly uses this 4 or 5 string set... but I wonder if we should start to add these kinds of overloads to AzureEventSource in core so that other libraries can use them - even just by coincidence. My rationale is that if we start to add these as they come up for individual needs, we'd be building out a reasonable common set.

Do you think that there's benefit to centralizing, or do you feel that we'd start to see too much bloat if we add all the special cases to the base? Regardless, the use is extensive enough in Event Hubs that there's definitely benefit to something like this.

[danielmarbach]

Assuming that I've got that correct, I'm good with the approach.

That is correct

Do you think that there's benefit to centralizing, or do you feel that we'd start to see too much bloat if we add all the special cases to the base? Regardless, the use is extensive enough in Event Hubs that there's definitely benefit to something like this.

I would need to know how many other event sources are actually using 4 and 5 string-based overloads. If there are many of those, it might warrant to centralize it. Doing that research is quite a bit of work for me, and I'm uncertain whether I can pull this off, honestly.

I would definitely not try to centralize the other permutations of overloads, since it would bloat the base class with highly specific use cases. WriteCore accepts the event data exactly to provide low allocation scenarios and for the not so common case I would go with that.

[jsquire]

I would need to know how many other event sources are actually using 4 and 5 string-based overloads. If there are many of those, it might warrant to centralize it.
...
I would definitely not try to centralize the other permutations of overloads, since it would bloat the base class with highly specific use cases.

Fair enough; I can't answer that either, and if we're trying to avoid bloating the general class, its probably better to just move forward with this as a special case locally.

[danielmarbach]

@jsquire I pushed a few examples how it would look like. Let me know if you want me to proceed with this. It will probably take a while until I got all the methods

[jsquire]

@jsquire I pushed a few examples how it would look like. Let me know if you want me to proceed with this. It will probably take a while until I got all the methods

Wow. I had mistakenly assumed that the scope was going to be a small handful of overrides. This seems like a non-trivial amount of work; if you're willing to keep going, please feel free. If you'd rather not, I want to be respectful of your kindness. I can add this to the backlog and schedule time to take over, if you'd prefer not to invest this level of time.

[danielmarbach]

I have found a good balance of generic methods covering the majority of the array allocations and a few specialized methods where necessary.

[danielmarbach]

This probably needs some good eyeballing to verify I haven't mixed up the parameter order, the event id or the data type

[jsquire]

Eyeball check looks good on the surface. I don't see anything obviously mismatched or count issues.

[jsquire]

Note to self:

• Move private members after public
• Add doc comments
• Refactor to move event identifiers into a new static class, EventHubsEventSourceEvents (or similar)
• Apply the pattern to EventProcessorClientEventSource
• Apply the pattern to BlobEventStoreEventSource
• Approach not needed for PartitionLoadBalancerEventSource, all events have 2 or less parameters.

[jsquire]

@danielmarbach: I left myself a bunch of notes for extending this. Once we think this is in a good stable spot, I'll probably grab a local copy of the changes and implement the first three bullets. That'll force me to give the parameter an event id matching a deeper look. We can then do a merge of this followed by my add-ons. Thoughts?

[jsquire]

/azp run net - eventhub - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[danielmarbach]

Works for me. The branch is open for contributions. That being said I'm also happy to help out. Right now though I'm a bit knocked out due to a sickness and don't have energy to do much.

If you start doing something locally just give me a quick heads up here (I'll do the same) so that we won't duplicate effort.

[danielmarbach]

@jsquire have a look. I pushed some things on the TODO list. I decided to copy the methods for now. Moving them to an extension method is not possible because we need access to the protected WriteEventCore method. The only other alternative would be to introduce another base class for all the event hub-specific event sources, but I felt copying the necessary method to the event sources that need them is the cleaner approach (at least to my taste :) )

I've also started outsourcing the event IDs. I did only five of them to see whether you like the approach or not. Once we have it confirmed, I can do a few more 🐒 style

[danielmarbach]

FYI ServiceBus has the constants in a region https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/servicebus/Azure.Messaging.ServiceBus/src/Diagnostics/ServiceBusEventSource.cs#L47

[danielmarbach]

The other alternative would be that the introduced *Core methods accept the eventId as a parameter. Then the locality would still be there and we don't have to move all the identifiers around.

[jsquire]

The other alternative would be that the introduced *Core methods accept the eventId as a parameter. Then the locality would still be there and we don't have to move all the identifiers around.

Oooh... I didn't think of that. I LOVE that idea.

[danielmarbach]

@jsquire Done. I think this is ready

[jsquire]

@jsquire Done. I think this is ready

Awesome. I'm going to pull down the change set locally and run some log captures via stress test to validate.
(the core logging was added very late in the release cycle and we do not have adequate unit tests for it; the newer areas have good test coverage and are passing!)

[jsquire]

@danielmarbach: I pushed a batch of minor formatting nits since you had previously mentioned that you wouldn't be offended if I did so. The only thing of substance that I did was to add the inlining hint to the two WriteEvent overloads that I commended on.

[danielmarbach]

LGTM

[jsquire]

Offline validation looks good. Some weird <null> characters that seem to appear only in file writes for events 6 and 7, but I confirmed those are appearing with the GA package as well, so unrelated to these changes. I'm going to give things one last eyeball tomorrow morning, and I think we're good to go.

[jsquire]

Overnight tests were successful. I think we're good to go.

@danielmarbach: Are you ready for me to merge or are there changes that you'd like to make?

[danielmarbach]