-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Avoid custom attribute routine in NetEventSource #120422
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
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @dotnet/ncl |
...ibraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/NetEventSource.WinHttpHandler.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes NetEventSource initialization across System.Net libraries by avoiding expensive GetCustomAttribute reflection calls during startup. The changes replace hardcoded string literals in EventSource attributes with constants and add explicit constructors that pass the event source name and settings directly to the base class.
Key changes:
- Add explicit constructors to NetEventSource classes that bypass attribute reflection
- Extract event source names to private constants for reusability
- Apply consistent pattern across all System.Net libraries
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
NetEventSource.WebSockets.cs | Add constructor and extract event source name constant |
NetEventSource.Sockets.cs | Add constructor and extract event source name constant |
NetEventSource.Security.cs | Add constructor and extract event source name constant |
NetEventSource.Requests.cs | Add constructor, extract constant, and expand class body |
NetEventSource.Quic.cs | Add constructor and extract event source name constant |
NetEventSource.Primitives.cs | Add constructor, extract constant, and expand class body |
NetEventSource.NetworkInformation.cs | Add constructor and extract event source name constant |
NetEventSource.NameResolution.cs | Add constructor, extract constant, and expand class body |
NetEventSource.Mail.cs | Add constructor, extract constant, and expand class body |
NetEventSource.HttpListener.cs | Add constructor, extract constant, and expand class body |
NetEventSource.Http.cs | Add constructor and extract event source name constant |
NetEventSource.WinHttpHandler.cs | Remove EventSource attribute and using statement |
...ibraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/NetEventSource.WinHttpHandler.cs
Outdated
Show resolved
Hide resolved
Nit: |
I'll ask my yearly question... are any of us still using these non-documented private event sources? There's always the option of deleting them outright :) |
Ideally, this would not require opt-in via an extra attribute and instead kick in automatically for all event sources when they are recompiled for a new TFM. |
I find them to be often useful when working with customers to diagnose issues. E.g. in #120179 it served as a replacement for network captures & decryption keys to narrow down what frames were being sent and whether pings were working. |
And from non-dotnet-team user's perspective - I've had fantastic use for them before! Just about a year ago, we've already used them to diagnose an issue in which, as it turned out, a dotnet http client disagreed with some Java service on whether the HTTP window size was exhausted or not. That was a bug in (Java) Akka's http stack, but the internal NET trace events were incredibly helpful to spot that. |
Apply @noahfalk's suggestion (here).
This allows us to avoid an expensive GetCustomAttribute routine that may have an impact on startup in certain scenarios (#120288)
Presumably, we can also pre-generate the Guid and save some cycles calling
GenerateGuidFromName
but that routine is a lot less expensive it seems.The right fix is to use
[EventSourceAutoGenerate]
source-gen which is currently SPC.dll onlyThe current change should be a little bit more trivial/friendly for a backport.