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

EventSource startup initialization #106014

Merged
merged 2 commits into from
Aug 8, 2024
Merged

Conversation

noahfalk
Copy link
Member

@noahfalk noahfalk commented Aug 6, 2024

Fixes #105845

Previously MetricsEventSource wasn't being created for apps that didn't ever create a Meter. This caused a chicken-and-egg problem for RuntimeMetrics which weren't created until MetricsEventSource started a tracing session. This change ensures that MetricsEventSource will be created on demand if ETW, EventPipe, or EventListener based tooling starts a tracing session. I took some extra effort to create the EventSource in a deferred fashion to avoid eager loading System.Diagnostics.DiagnosticSource.dll when it might never be needed.

Aside from the fix there were some small improvements:

  • Moved NativeRuntimeEventSource to initialize in the same place as other startup EventSources
  • Removed a useless lock(EventListener.EventListenersLock) around EventPipe eventProvider registration

Fixes dotnet#105845

Previously MetricsEventSource wasn't being created for apps that didn't ever create a Meter. This caused a chicken-and-egg problem for RuntimeMetrics which weren't created until MetricsEventSource started a tracing session. This change ensures that MetricsEventSource will be created on demand if ETW, EventPipe, or EventListener based tooling starts a tracing session. I took some extra effort to create the EventSource in a deferred fashion to avoid eager loading System.Diagnostics.DiagnosticSource.dll when it might never be needed.

Aside from the fix there were some small improvements:
- Moved NativeRuntimeEventSource to initialize in the same place as other startup EventSources
- Removed a useless lock(EventListener.EventListenersLock) around EventPipe eventProvider registration
@noahfalk
Copy link
Member Author

noahfalk commented Aug 7, 2024

I think this is ready for review now. The CI failures appear spurious.
@tarekgh @brianrob @dotnet/dotnet-diag @lambdageek @lateralusX

@noahfalk
Copy link
Member Author

noahfalk commented Aug 7, 2024

@LakshanF @MichalStrehovsky - this change also adjusted the NativeAOT startup slightly by moving the ModuleInitializer attribute from RuntimeEventSource.Initialize() to EventSource.InitializeDefaultEventSources().

@elinor-fung - this change adjusts the startup path you were just editing earlier this week.

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@brianrob
Copy link
Member

brianrob commented Aug 8, 2024

I suspect that I just missed something here, but want to check - I see where you register the listeners for ETW and EventPipe and trigger creation if/when a command comes in. How is EventListener handled? Something will need to create the MetricsEventSource so that OnEventSourceCreated gets called. I'm assuming you have this covered and I just missed it.

@noahfalk
Copy link
Member Author

noahfalk commented Aug 8, 2024

How is EventListener handled? Something will need to create the MetricsEventSource so that OnEventSourceCreated gets called. I'm assuming you have this covered and I just missed it.

Yep, thats this part right here to handle EventListener callbacks:
https://github.com/dotnet/runtime/pull/106014/files#diff-ebcaac338c0ea28059b6d78c1d8725941f461bb7d6b08d83024c3f873ce012a1R4579-R4581

@MichalStrehovsky
Copy link
Member

@LakshanF @MichalStrehovsky - this change also adjusted the NativeAOT startup slightly by moving the ModuleInitializer attribute from RuntimeEventSource.Initialize() to EventSource.InitializeDefaultEventSources().

This means any app with EventSourceSupport=true will pay for this in terms of size, but it doesn't look terrible. I triggered a rt-sz run and it seems like we'll have a 30 kB regression in dotnet new webapiaot: MichalStrehovsky/rt-sz#60 (comment). Most of this cost actually comes from Type.GetType since nothing else uses that API.

It doesn't make a huge difference in EventSource cost (dotnet new webapiaot followed by dotnet publish vs dotnet publish -p:EventSourceSupport=false shows EventSourceSupport increases size by 11%, or 1 MB).

@brianrob
Copy link
Member

brianrob commented Aug 8, 2024

How is EventListener handled? Something will need to create the MetricsEventSource so that OnEventSourceCreated gets called. I'm assuming you have this covered and I just missed it.

Yep, thats this part right here to handle EventListener callbacks: https://github.com/dotnet/runtime/pull/106014/files#diff-ebcaac338c0ea28059b6d78c1d8725941f461bb7d6b08d83024c3f873ce012a1R4579-R4581

Thanks! LGTM.

@noahfalk
Copy link
Member Author

noahfalk commented Aug 8, 2024

Thanks for the feedback, it should all be addressed now. Once CI confirms no new issues I'll merge.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initializing RuntimeMetrics
6 participants