Skip to content

Reduce allocations when Activity is enabled #11020

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

Merged
merged 3 commits into from
Jun 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 24 additions & 3 deletions src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ internal class HostingApplicationDiagnostics
private static readonly double TimestampToTicks = TimeSpan.TicksPerSecond / (double)Stopwatch.Frequency;

private const string ActivityName = "Microsoft.AspNetCore.Hosting.HttpRequestIn";
private const string ActivityStartKey = "Microsoft.AspNetCore.Hosting.HttpRequestIn.Start";
private const string ActivityStartKey = ActivityName + ".Start";
private const string ActivityStopKey = ActivityName + ".Stop";

private const string DeprecatedDiagnosticsBeginRequestKey = "Microsoft.AspNetCore.Hosting.BeginRequest";
private const string DeprecatedDiagnosticsEndRequestKey = "Microsoft.AspNetCore.Hosting.EndRequest";
Expand Down Expand Up @@ -278,7 +279,7 @@ private Activity StartActivity(HttpContext httpContext, out bool hasDiagnosticLi
if (_diagnosticListener.IsEnabled(ActivityStartKey))
{
hasDiagnosticListener = true;
_diagnosticListener.StartActivity(activity, new { HttpContext = httpContext });
StartActivity(activity, httpContext);
Copy link

@lmolkova lmolkova Jun 14, 2019

Choose a reason for hiding this comment

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

I like this and we may be able to update listener in time for 3.0 to support it. But it is breaking (i.e. there is no HttpContext property on HttpContext type), everyone who attempts to get it will fail.

Copy link

@lmolkova lmolkova Jun 14, 2019

Choose a reason for hiding this comment

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

ooops, you've added property on HttpConetxt 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

HttpContext.HttpContext 😄

}
else
{
Expand All @@ -293,12 +294,32 @@ private void StopActivity(HttpContext httpContext, Activity activity, bool hasDi
{
if (hasDiagnosticListener)
{
_diagnosticListener.StopActivity(activity, new { HttpContext = httpContext });
StopActivity(activity, httpContext);
}
else
{
activity.Stop();
}
}

// These are versions of DiagnosticSource.Start/StopActivity that don't allocate strings per call (see https://github.com/dotnet/corefx/issues/37055)
private Activity StartActivity(Activity activity, HttpContext httpContext)
{
activity.Start();
_diagnosticListener.Write(ActivityStartKey, httpContext);
return activity;
}

private void StopActivity(Activity activity, HttpContext httpContext)
{
// Stop sets the end time if it was unset, but we want it set before we issue the write
// so we do it now.
if (activity.Duration == TimeSpan.Zero)
{
activity.SetEndTime(DateTime.UtcNow);
}
_diagnosticListener.Write(ActivityStopKey, httpContext);
activity.Stop(); // Resets Activity.Current (we want this after the Write)
}
}
}
2 changes: 2 additions & 0 deletions src/Http/Http/ref/Microsoft.AspNetCore.Http.netcoreapp3.0.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ public DefaultHttpContext(Microsoft.AspNetCore.Http.Features.IFeatureCollection
public override Microsoft.AspNetCore.Http.ConnectionInfo Connection { get { throw null; } }
public override Microsoft.AspNetCore.Http.Features.IFeatureCollection Features { get { throw null; } }
public Microsoft.AspNetCore.Http.Features.FormOptions FormOptions { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
public Microsoft.AspNetCore.Http.HttpContext HttpContext { get { throw null; } }
public override System.Collections.Generic.IDictionary<object, object> Items { get { throw null; } set { } }
public override Microsoft.AspNetCore.Http.HttpRequest Request { get { throw null; } }
public override System.Threading.CancellationToken RequestAborted { get { throw null; } set { } }
Expand Down
7 changes: 7 additions & 0 deletions src/Http/Http/src/DefaultHttpContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Security.Claims;
using System.Threading;
using Microsoft.AspNetCore.Http.Features;
Expand Down Expand Up @@ -157,6 +158,12 @@ public override ISession Session
}
}

// This property exists because of backwards compatibility.
// We send an anonymous object with an HttpContext property
// via DiagnosticSource in various events throughout the pipeline. Instead
// we just send the HttpContext to avoid extra allocations
[EditorBrowsable(EditorBrowsableState.Never)]
public HttpContext HttpContext => this;

public override void Abort()
{
Expand Down