Add feature switch for diagnostics handler in System.Net.Http #38765
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
Tagging subscribers to this area: @dotnet/ncl |
| @@ -14,6 +14,15 @@ | |||
| <PropertyGroup Condition=" '$(TargetsBrowser)' == 'true'"> | |||
| <DefineConstants>$(DefineConstants);TARGETS_BROWSER</DefineConstants> | |||
| </PropertyGroup> | |||
| <!-- ILLinker settings --> | |||
There was a problem hiding this comment.
Do we have same docs how this linker stuff works? Could you point me to it?
I'd like to understand these changes better.
There was a problem hiding this comment.
@eerhardt wrote the build task but I'm not sure there is any doc. You can check the code at https://github.com/dotnet/runtime/blob/master/eng/illink.targets
There was a problem hiding this comment.
A good starting overview is: https://github.com/dotnet/designs/blob/master/accepted/2020/linking-libraries.md
The feature switch design is good at explaining how certain code can be trimmed when a user doesn't want it: https://github.com/dotnet/designs/blob/master/accepted/2020/feature-switch.md.
|
Cc: @alnikola @MihaZupan this is touching diagnostics, that's your domain guys. |
| <linker> | ||
| <assembly fullname="System.Net.Http"> | ||
| <type fullname="System.Net.Http.DiagnosticsHandler"> | ||
| <method signature="System.Boolean IsEnabled()" body="stub" value="false" /> |
There was a problem hiding this comment.
Why not make this into a full blown feature switch? There is already an AppContext setting.
Advantages:
- If someone wants to turn it off, they can by setting a single setting in their .csproj, just like any other feature switch.
- It can easily be used in other size-sensitive form factors.
There was a problem hiding this comment.
Changed the substitution to full feature switch
| <ILLinkDirectory>$(MSBuildThisFileDirectory)src\ILLink\</ILLinkDirectory> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <ILLinkSubstitutionsXmls Include="$(ILLinkDirectory)ILLink.Substitutions.$(Platform).xml" |
There was a problem hiding this comment.
I don't think $(Platform) works here. We only build per-platform binaries for System.Private.CoreLib. All other libraries are cross compiled by TFM and OS, but not by architecture.
If we use a full feature switch, this will go away as the Blazor SDK can just default the MSBuild setting to add --feature System.Net.Http.DiagnosticsHandler.IsSupported false when running the linker.
marek-safar
changed the title
…gation These feature switches were added in the runtime: * Debugger - dotnet/runtime#37288 * Http activity propagation - dotnet/runtime#38765 Fix dotnet#12217
…gation (#12457) These feature switches were added in the runtime: * Debugger - dotnet/runtime#37288 * Http activity propagation - dotnet/runtime#38765 Fix #12217
Since the DiagnosticsHandler still gets instantiated in the HttpClientHandler, none of its overriden methods are getting trimmed. This leads to System.Diagnostics.DiagnosticListener still being preserved in a linked application. This fix is split DiagnosticsHandler.IsEnabled() into two methods: * IsGloballyEnabled() - checks the AppContext switch, and is replaced by the linker by a feature swtich. * IsEnabled() - which checks IsGloballyEnabled() and if there is an Activity or listener available. This allows all but the IsEnabled and IsGloballyEnabled methods to get trimmed on DiagnosticsHandler. Contributes to dotnet#38765
Since the DiagnosticsHandler still gets instantiated in the HttpClientHandler, none of its overriden methods are getting trimmed. This leads to System.Diagnostics.DiagnosticListener still being preserved in a linked application. The fix is to split DiagnosticsHandler.IsEnabled() into two methods: * IsGloballyEnabled() - checks the AppContext switch, and is replaced by the linker by a feature swtich. * IsEnabled() - which checks IsGloballyEnabled() and if there is an Activity or listener available. This allows all but the IsEnabled and IsGloballyEnabled methods to get trimmed on DiagnosticsHandler. Contributes to #38765
Since the DiagnosticsHandler still gets instantiated in the HttpClientHandler, none of its overriden methods are getting trimmed. This leads to System.Diagnostics.DiagnosticListener still being preserved in a linked application. The fix is to split DiagnosticsHandler.IsEnabled() into two methods: * IsGloballyEnabled() - checks the AppContext switch, and is replaced by the linker by a feature swtich. * IsEnabled() - which checks IsGloballyEnabled() and if there is an Activity or listener available. This allows all but the IsEnabled and IsGloballyEnabled methods to get trimmed on DiagnosticsHandler. Contributes to dotnet#38765
ghost
locked as resolved and limited conversation to collaborators
This allows easier exclusion on size sensitive workloads