Skip to content

Commit

Permalink
Trim more Http DiagnosticsHandler code (#39525)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
eerhardt authored Jul 21, 2020
1 parent d129af6 commit 632cdca
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 7 deletions.
2 changes: 1 addition & 1 deletion docs/workflow/trimming/feature-switches.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<linker>
<assembly fullname="System.Net.Http">
<type fullname="System.Net.Http.DiagnosticsHandler">
<method signature="System.Boolean IsEnabled()" body="stub" value="false" feature="System.Net.Http.EnableActivityPropagation" featurevalue="false" />
<method signature="System.Boolean IsGloballyEnabled()" body="stub" value="false" feature="System.Net.Http.EnableActivityPropagation" featurevalue="false" />
</type>
</assembly>
</linker>
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}

Expand Down Expand Up @@ -273,15 +276,15 @@ 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);
}

protected internal override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request,
CancellationToken cancellationToken)
{
return DiagnosticsHandler.IsEnabled() ?
return DiagnosticsHandler.IsEnabled() && _diagnosticsHandler != null ?
_diagnosticsHandler.SendAsync(request, cancellationToken) :
_underlyingHandler.SendAsync(request, cancellationToken);
}
Expand Down

0 comments on commit 632cdca

Please sign in to comment.