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.

Sign up for GitHub

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

Add feature switch for diagnostics handler in System.Net.Http #38765

Merged
merged 5 commits into from
Jul 9, 2020
Merged

Add feature switch for diagnostics handler in System.Net.Http #38765

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7189df4
Exclude diagnostics handler from browser version of System.Net.Http
marek-safar Jul 3, 2020
9ec9a4e
Update System.Net.Http.csproj
marek-safar Jul 7, 2020
07c555d
Add feature switch to substitutions
marek-safar Jul 8, 2020
ae7c356
Merge branch 'http-wasm' of github.com:marek-safar/runtime into http-…
marek-safar Jul 8, 2020
9749e06
Update System.Net.Http.csproj
marek-safar Jul 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/libraries/System.Net.Http/src/ILLink/ILLink.Substitutions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +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" />
</type>
</assembly>
</linker>
12 changes: 10 additions & 2 deletions src/libraries/System.Net.Http/src/System.Net.Http.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@
<PropertyGroup Condition=" '$(TargetsBrowser)' == 'true'">
<DefineConstants>$(DefineConstants);TARGETS_BROWSER</DefineConstants>
</PropertyGroup>
<!-- ILLinker settings -->
Copy link
Member

Choose a reason for hiding this comment

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

Do we have same docs how this linker stuff works? Could you point me to it?
I'd like to understand these changes better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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.

<PropertyGroup>
<ILLinkDirectory>$(MSBuildThisFileDirectory)src\ILLink\</ILLinkDirectory>
</PropertyGroup>
<ItemGroup>
<ILLinkSubstitutionsXmls Include="$(ILLinkDirectory)ILLink.Substitutions.xml" />
</ItemGroup>

<ItemGroup>
<Compile Include="System\Net\Http\ByteArrayContent.cs" />
<Compile Include="System\Net\Http\ByteArrayHelpers.cs" />
Expand Down Expand Up @@ -337,7 +345,7 @@
<Compile Include="$(CommonPath)System\Net\Http\aspnetcore\Http3\QPack\H3StaticTable.cs">
<Link>Common\System\Net\Http\aspnetcore\Http3\QPack\H3StaticTable.cs</Link>
</Compile>
</ItemGroup>
</ItemGroup>
<!-- Linux specific files -->
<ItemGroup Condition="'$(TargetsLinux)' == 'true' ">
<Compile Include="$(CommonPath)Interop\Linux\Interop.Libraries.cs"
Expand Down Expand Up @@ -731,5 +739,5 @@
</ItemGroup>
<ItemGroup Condition=" '$(TargetsBrowser)' == 'true'">
<ProjectReference Include="..\..\System.Runtime.InteropServices.JavaScript/src/System.Runtime.InteropServices.JavaScript.csproj" />
</ItemGroup>
</ItemGroup>
</Project>
68 changes: 36 additions & 32 deletions src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ internal static bool IsEnabled()
{
// check if there is a parent Activity (and propagation is not suppressed)
// or if someone listens to HttpHandlerDiagnosticListener
return s_enableActivityPropagation && (Activity.Current != null || s_diagnosticListener.IsEnabled());
return Settings.s_activityPropagationEnabled && (Activity.Current != null || Settings.s_diagnosticListener.IsEnabled());
}

// SendAsyncCore returns already completed ValueTask for when async: false is passed.
Expand Down Expand Up @@ -59,10 +59,11 @@ private async ValueTask<HttpResponseMessage> SendAsyncCore(HttpRequestMessage re
}

Activity? activity = null;
DiagnosticListener diagnosticListener = Settings.s_diagnosticListener;

// if there is no listener, but propagation is enabled (with previous IsEnabled() check)
// do not write any events just start/stop Activity and propagate Ids
if (!s_diagnosticListener.IsEnabled())
if (!diagnosticListener.IsEnabled())
{
activity = new Activity(DiagnosticsHandlerLoggingStrings.ActivityName);
activity.Start();
Expand All @@ -83,26 +84,26 @@ await base.SendAsync(request, cancellationToken).ConfigureAwait(false) :
Guid loggingRequestId = Guid.Empty;

// There is a listener. Check if listener wants to be notified about HttpClient Activities
if (s_diagnosticListener.IsEnabled(DiagnosticsHandlerLoggingStrings.ActivityName, request))
if (diagnosticListener.IsEnabled(DiagnosticsHandlerLoggingStrings.ActivityName, request))
{
activity = new Activity(DiagnosticsHandlerLoggingStrings.ActivityName);

// Only send start event to users who subscribed for it, but start activity anyway
if (s_diagnosticListener.IsEnabled(DiagnosticsHandlerLoggingStrings.ActivityStartName))
if (diagnosticListener.IsEnabled(DiagnosticsHandlerLoggingStrings.ActivityStartName))
{
s_diagnosticListener.StartActivity(activity, new ActivityStartData(request));
diagnosticListener.StartActivity(activity, new ActivityStartData(request));
}
else
{
activity.Start();
}
}
// try to write System.Net.Http.Request event (deprecated)
if (s_diagnosticListener.IsEnabled(DiagnosticsHandlerLoggingStrings.RequestWriteNameDeprecated))
if (diagnosticListener.IsEnabled(DiagnosticsHandlerLoggingStrings.RequestWriteNameDeprecated))
{
long timestamp = Stopwatch.GetTimestamp();
loggingRequestId = Guid.NewGuid();
s_diagnosticListener.Write(DiagnosticsHandlerLoggingStrings.RequestWriteNameDeprecated,
diagnosticListener.Write(DiagnosticsHandlerLoggingStrings.RequestWriteNameDeprecated,
new RequestData(request, loggingRequestId, timestamp));
}

Expand Down Expand Up @@ -133,12 +134,12 @@ await base.SendAsync(request, cancellationToken).ConfigureAwait(false) :
{
taskStatus = TaskStatus.Faulted;

if (s_diagnosticListener.IsEnabled(DiagnosticsHandlerLoggingStrings.ExceptionEventName))
if (diagnosticListener.IsEnabled(DiagnosticsHandlerLoggingStrings.ExceptionEventName))
{
// If request was initially instrumented, Activity.Current has all necessary context for logging
// Request is passed to provide some context if instrumentation was disabled and to avoid
// extensive Activity.Tags usage to tunnel request properties
s_diagnosticListener.Write(DiagnosticsHandlerLoggingStrings.ExceptionEventName, new ExceptionData(ex, request));
diagnosticListener.Write(DiagnosticsHandlerLoggingStrings.ExceptionEventName, new ExceptionData(ex, request));
}
throw;
}
Expand All @@ -147,7 +148,7 @@ await base.SendAsync(request, cancellationToken).ConfigureAwait(false) :
// always stop activity if it was started
if (activity != null)
{
s_diagnosticListener.StopActivity(activity, new ActivityStopData(
diagnosticListener.StopActivity(activity, new ActivityStopData(
response,
// If request is failed or cancelled, there is no response, therefore no information about request;
// pass the request in the payload, so consumers can have it in Stop for failed/canceled requests
Expand All @@ -156,10 +157,10 @@ await base.SendAsync(request, cancellationToken).ConfigureAwait(false) :
taskStatus));
}
// Try to write System.Net.Http.Response event (deprecated)
if (s_diagnosticListener.IsEnabled(DiagnosticsHandlerLoggingStrings.ResponseWriteNameDeprecated))
if (diagnosticListener.IsEnabled(DiagnosticsHandlerLoggingStrings.ResponseWriteNameDeprecated))
{
long timestamp = Stopwatch.GetTimestamp();
s_diagnosticListener.Write(DiagnosticsHandlerLoggingStrings.ResponseWriteNameDeprecated,
diagnosticListener.Write(DiagnosticsHandlerLoggingStrings.ResponseWriteNameDeprecated,
new ResponseData(
response,
loggingRequestId,
Expand Down Expand Up @@ -247,29 +248,35 @@ internal ResponseData(HttpResponseMessage? response, Guid loggingRequestId, long
public override string ToString() => $"{{ {nameof(Response)} = {Response}, {nameof(LoggingRequestId)} = {LoggingRequestId}, {nameof(Timestamp)} = {Timestamp}, {nameof(RequestTaskStatus)} = {RequestTaskStatus} }}";
}

private const string EnableActivityPropagationEnvironmentVariableSettingName = "DOTNET_SYSTEM_NET_HTTP_ENABLEACTIVITYPROPAGATION";
private const string EnableActivityPropagationAppCtxSettingName = "System.Net.Http.EnableActivityPropagation";
private static class Settings
{
private const string EnableActivityPropagationEnvironmentVariableSettingName = "DOTNET_SYSTEM_NET_HTTP_ENABLEACTIVITYPROPAGATION";
private const string EnableActivityPropagationAppCtxSettingName = "System.Net.Http.EnableActivityPropagation";

private static readonly bool s_enableActivityPropagation = GetEnableActivityPropagationValue();
public static readonly bool s_activityPropagationEnabled = GetEnableActivityPropagationValue();

private static bool GetEnableActivityPropagationValue()
{
// First check for the AppContext switch, giving it priority over the environment variable.
if (AppContext.TryGetSwitch(EnableActivityPropagationAppCtxSettingName, out bool enableActivityPropagation))
private static bool GetEnableActivityPropagationValue()
{
return enableActivityPropagation;
}
// First check for the AppContext switch, giving it priority over the environment variable.
if (AppContext.TryGetSwitch(EnableActivityPropagationAppCtxSettingName, out bool enableActivityPropagation))
{
return enableActivityPropagation;
}

// AppContext switch wasn't used. Check the environment variable to determine which handler should be used.
string? envVar = Environment.GetEnvironmentVariable(EnableActivityPropagationEnvironmentVariableSettingName);
if (envVar != null && (envVar.Equals("false", StringComparison.OrdinalIgnoreCase) || envVar.Equals("0")))
{
// Suppress Activity propagation.
return false;
// AppContext switch wasn't used. Check the environment variable to determine which handler should be used.
string? envVar = Environment.GetEnvironmentVariable(EnableActivityPropagationEnvironmentVariableSettingName);
if (envVar != null && (envVar.Equals("false", StringComparison.OrdinalIgnoreCase) || envVar.Equals("0")))
{
// Suppress Activity propagation.
return false;
}

// Defaults to enabling Activity propagation.
return true;
}

// Defaults to enabling Activity propagation.
return true;
public static readonly DiagnosticListener s_diagnosticListener =
new DiagnosticListener(DiagnosticsHandlerLoggingStrings.DiagnosticListenerName);
}

private static void InjectHeaders(Activity currentActivity, HttpRequestMessage request)
Expand Down Expand Up @@ -310,9 +317,6 @@ private static void InjectHeaders(Activity currentActivity, HttpRequestMessage r
}
}

private static readonly DiagnosticListener s_diagnosticListener =
new DiagnosticListener(DiagnosticsHandlerLoggingStrings.DiagnosticListenerName);

#endregion
}
}