Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Utilize formatted message from AzureEventSourceListener in LogEvent #44048

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

christothes
Copy link
Member

closes #44028

@lmolkova - If this fix makes sense, there are other files where we should make this fix.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@pharring
Copy link
Contributor

I think the desire in the original code is to defer formatting the message until/unless it's absolutely needed. In particular, you want to avoid (expensive) formatting if the log level would otherwise filter the event entirely.

I would propose an alternative where we add a second (overloaded) constructor to AzureEventSourceListener that takes a simpler Action<EventWrittenArgs>. Callers that don't need the formatted message can use that constructor.

See this: https://github.com/pharring/azure-sdk-for-net/compare/dev/pharring/EventSourceEventFormattingPerf...pharring:azure-sdk-for-net:dev/pharring/AddAzureEventSourceListenerConstructorOverload?expand=1

@christothes
Copy link
Member Author

I think the desire in the original code is to defer formatting the message until/unless it's absolutely needed. In particular, you want to avoid (expensive) formatting if the log level would otherwise filter the event entirely.

I would propose an alternative where we add a second (overloaded) constructor to AzureEventSourceListener that takes a simpler Action<EventWrittenArgs>. Callers that don't need the formatted message can use that constructor.

See this: https://github.com/pharring/azure-sdk-for-net/compare/dev/pharring/EventSourceEventFormattingPerf...pharring:azure-sdk-for-net:dev/pharring/AddAzureEventSourceListenerConstructorOverload?expand=1

As I read it, the formatting doesn't happen until the event is logged either way. Whether it happens higher or lower in the stack seems immaterial. But perhaps I am missing something?

@pharring
Copy link
Contributor

Yes, you are missing something. In this PR, message formatting (likely a string allocation) still happens unconditionally inside AzureEventSourceListener in order to evaluate the arguments to call the LogEvent callback, passing the fully-evaluated formattedMessage to LogEvent. If the log level is set high/low enough that the event will be filtered, then the fully formatted message is never used -- meaning it was a wasted allocation.

@christothes
Copy link
Member Author

Does the EventSource not avoid the eager formatting when it calls IsEnabled before calling WriteEvent?

@pharring
Copy link
Contributor

Does the EventSource not avoid the eager formatting when it calls IsEnabled before calling WriteEvent?

Yes, it does. However, this listener subscribes to events at "Verbose" level (meaning everything) and the AzureEventSourceListener uses calls EnableEvents without passing a keyword mask -- also meaning "all events".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Monitor - Distro Monitor OpenTelemetry Distro
Projects
None yet
3 participants