From 632cdcab09fda363e460296a77be0be32096f512 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Tue, 21 Jul 2020 08:59:52 -0500 Subject: [PATCH] Trim more Http DiagnosticsHandler code (#39525) 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 --- docs/workflow/trimming/feature-switches.md | 2 +- .../src/ILLink/ILLink.Substitutions.xml | 2 +- .../src/System/Net/Http/DiagnosticsHandler.cs | 7 ++++++- .../src/System/Net/Http/HttpClientHandler.cs | 11 +++++++---- 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/docs/workflow/trimming/feature-switches.md b/docs/workflow/trimming/feature-switches.md index ad911e818d7a53..40d3a2299f474b 100644 --- a/docs/workflow/trimming/feature-switches.md +++ b/docs/workflow/trimming/feature-switches.md @@ -14,7 +14,7 @@ configurations but their defaults might vary as any SDK can set the defaults dif | EventSourceSupport | System.Diagnostics.Tracing.EventSource.IsSupported | Any EventSource related code or logic is trimmed when set to false | | InvariantGlobalization | System.Globalization.Invariant | All globalization specific code and data is trimmed when set to true | | UseSystemResourceKeys | System.Resources.UseSystemResourceKeys | Any localizable resources for system assemblies is trimmed when set to true | -| - | System.Net.Http.EnableActivityPropagation | Any dependency related to diagnostics support for System.Net.Http is trimmed when set to false | +| HttpActivityPropagationSupport | System.Net.Http.EnableActivityPropagation | Any dependency related to diagnostics support for System.Net.Http is trimmed when set to false | Any feature-switch which defines property can be set in csproj file or on the command line as any other MSBuild property. Those without predefined property name diff --git a/src/libraries/System.Net.Http/src/ILLink/ILLink.Substitutions.xml b/src/libraries/System.Net.Http/src/ILLink/ILLink.Substitutions.xml index 5715e75fc8303a..5e70f6fbb5d7d5 100644 --- a/src/libraries/System.Net.Http/src/ILLink/ILLink.Substitutions.xml +++ b/src/libraries/System.Net.Http/src/ILLink/ILLink.Substitutions.xml @@ -1,7 +1,7 @@ - + diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs index f69d5ff2f77789..47f260eec04b03 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs @@ -27,7 +27,12 @@ internal static bool IsEnabled() { // check if there is a parent Activity (and propagation is not suppressed) // or if someone listens to HttpHandlerDiagnosticListener - return Settings.s_activityPropagationEnabled && (Activity.Current != null || Settings.s_diagnosticListener.IsEnabled()); + return IsGloballyEnabled() && (Activity.Current != null || Settings.s_diagnosticListener.IsEnabled()); + } + + internal static bool IsGloballyEnabled() + { + return Settings.s_activityPropagationEnabled; } // SendAsyncCore returns already completed ValueTask for when async: false is passed. diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs index ec2e729113b18b..996e42ca02c36d 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs @@ -18,7 +18,7 @@ public partial class HttpClientHandler : HttpMessageHandler #else private readonly SocketsHttpHandler _underlyingHandler; #endif - private readonly DiagnosticsHandler _diagnosticsHandler; + private readonly DiagnosticsHandler? _diagnosticsHandler; private ClientCertificateOption _clientCertificateOptions; private volatile bool _disposed; @@ -30,7 +30,10 @@ public HttpClientHandler() #else _underlyingHandler = new SocketsHttpHandler(); #endif - _diagnosticsHandler = new DiagnosticsHandler(_underlyingHandler); + if (DiagnosticsHandler.IsGloballyEnabled()) + { + _diagnosticsHandler = new DiagnosticsHandler(_underlyingHandler); + } ClientCertificateOptions = ClientCertificateOption.Manual; } @@ -273,7 +276,7 @@ public SslProtocols SslProtocols protected internal override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken) { - return DiagnosticsHandler.IsEnabled() ? + return DiagnosticsHandler.IsEnabled() && _diagnosticsHandler != null ? _diagnosticsHandler.Send(request, cancellationToken) : _underlyingHandler.Send(request, cancellationToken); } @@ -281,7 +284,7 @@ protected internal override HttpResponseMessage Send(HttpRequestMessage request, protected internal override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { - return DiagnosticsHandler.IsEnabled() ? + return DiagnosticsHandler.IsEnabled() && _diagnosticsHandler != null ? _diagnosticsHandler.SendAsync(request, cancellationToken) : _underlyingHandler.SendAsync(request, cancellationToken); }