Skip to content

Diaganostics and metrics clean up #47679

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 6 commits into from
Apr 14, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
WIP
  • Loading branch information
JamesNK committed Apr 13, 2023
commit 5f851a9db8e2f484f48fa3598d047efbfb34d000
7 changes: 5 additions & 2 deletions src/Hosting/Hosting/src/Internal/HostingApplication.cs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,10 @@ public Activity? Activity
{
if (HttpActivityFeature is null)
{
HttpActivityFeature = new ActivityFeature(value!);
if (value != null)
{
HttpActivityFeature = new ActivityFeature(value);
}
}
else
{
Expand All @@ -144,7 +147,7 @@ public Activity? Activity
public bool EventLogOrMetricsEnabled { get; set; }

internal IHttpActivityFeature? HttpActivityFeature;
internal HttpMetricsTagsFeature? MetricsTagsFeature;
internal IHttpMetricsTagsFeature? MetricsTagsFeature;

public void Reset()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@ public void BeginRequest(HttpContext httpContext, HostingApplication.Context con
if (_eventSource.IsEnabled() || _metrics.IsEnabled())
{
context.EventLogOrMetricsEnabled = true;
if (httpContext.Features.Get<IHttpMetricsTagsFeature>() is HttpMetricsTagsFeature feature)
if (httpContext.Features.Get<IHttpMetricsTagsFeature>() is IHttpMetricsTagsFeature feature)
{
context.MetricsTagsFeature = feature;
}
else
{
context.MetricsTagsFeature ??= new HttpMetricsTagsFeature();
httpContext.Features.Set<IHttpMetricsTagsFeature>(context.MetricsTagsFeature);
httpContext.Features.Set(context.MetricsTagsFeature);
}

startTimestamp = Stopwatch.GetTimestamp();
Expand Down Expand Up @@ -146,7 +146,7 @@ public void RequestEnd(HttpContext httpContext, Exception? exception, HostingApp
if (context.EventLogOrMetricsEnabled)
{
var route = httpContext.GetEndpoint()?.Metadata.GetMetadata<IRouteDiagnosticsMetadata>()?.Route;
var customTags = context.MetricsTagsFeature?.TagsList;
var customTags = context.MetricsTagsFeature?.Tags;

_metrics.RequestEnd(
httpContext.Request.Protocol,
Expand Down
26 changes: 21 additions & 5 deletions src/Hosting/Hosting/src/Internal/HostingMetrics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public void RequestStart(bool isHttps, string scheme, string method, HostString
_currentRequestsCounter.Add(1, tags);
}

public void RequestEnd(string protocol, bool isHttps, string scheme, string method, HostString host, string? route, int statusCode, Exception? exception, List<KeyValuePair<string, object?>>? customTags, long startTimestamp, long currentTimestamp)
public void RequestEnd(string protocol, bool isHttps, string scheme, string method, HostString host, string? route, int statusCode, Exception? exception, ICollection<KeyValuePair<string, object?>>? customTags, long startTimestamp, long currentTimestamp)
{
var tags = new TagList();
InitializeRequestTags(ref tags, isHttps, scheme, method, host);
Expand Down Expand Up @@ -69,17 +69,33 @@ public void RequestEnd(string protocol, bool isHttps, string scheme, string meth
}
if (customTags != null)
{
for (var i = 0; i < customTags.Count; i++)
{
tags.Add(customTags[i]);
}
AddCustomTags(ref tags, customTags);
}

var duration = Stopwatch.GetElapsedTime(startTimestamp, currentTimestamp);
_requestDuration.Record(duration.TotalSeconds, tags);
}
}

private static void AddCustomTags(ref TagList tags, ICollection<KeyValuePair<string, object?>> customTags)
{
// Skip allocating enumerator if tags collection is a list.
if (customTags is List<KeyValuePair<string, object?>> list)
{
for (var i = 0; i < list.Count; i++)
{
tags.Add(list[i]);
}
}
else
{
foreach (var tag in customTags)
{
tags.Add(tag);
}
}
}

public void Dispose()
{
_meter.Dispose();
Expand Down
4 changes: 1 addition & 3 deletions src/Hosting/Hosting/src/Internal/HttpMetricsTagsFeature.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,5 @@ namespace Microsoft.AspNetCore.Hosting;

internal sealed class HttpMetricsTagsFeature : IHttpMetricsTagsFeature
{
public ICollection<KeyValuePair<string, object?>> Tags => TagsList;

public List<KeyValuePair<string, object?>> TagsList { get; } = new List<KeyValuePair<string, object?>>();
public ICollection<KeyValuePair<string, object?>> Tags { get; } = new List<KeyValuePair<string, object?>>();
}
49 changes: 47 additions & 2 deletions src/Hosting/Hosting/test/HostingApplicationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
using System.Collections.ObjectModel;
using System.Diagnostics;
using System.Diagnostics.Metrics;
using Microsoft.AspNetCore.Hosting.Fakes;
Expand Down Expand Up @@ -108,6 +109,46 @@ static void AssertRequestDuration(Measurement<double> measurement, string protoc
}
}

[Fact]
public void MetricsFeatureUsedFromFeatureCollection()
{
// Arrange
var meterFactory = new TestMeterFactory();
var meterRegistry = new TestMeterRegistry(meterFactory.Meters);
var hostingApplication = CreateApplication(meterFactory: meterFactory);
var httpContext = new DefaultHttpContext();
var meter = meterFactory.Meters.Single();

using var requestDurationRecorder = new InstrumentRecorder<double>(meterRegistry, HostingMetrics.MeterName, "request-duration");
using var currentRequestsRecorder = new InstrumentRecorder<long>(meterRegistry, HostingMetrics.MeterName, "current-requests");

// Act/Assert
Assert.Equal(HostingMetrics.MeterName, meter.Name);
Assert.Null(meter.Version);

var feature = new TestHttpMetricsTagsFeature();
feature.Tags.Add(new KeyValuePair<string, object>("test", "Value!"));
httpContext.Features.Set<IHttpMetricsTagsFeature>(feature);
var context1 = hostingApplication.CreateContext(httpContext.Features);
context1.HttpContext.Response.StatusCode = StatusCodes.Status200OK;
hostingApplication.DisposeContext(context1, null);

Assert.Collection(currentRequestsRecorder.GetMeasurements(),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value));
Assert.Collection(requestDurationRecorder.GetMeasurements(),
m =>
{
Assert.True(m.Value > 0);
Assert.Equal("Value!", (string)m.Tags.ToArray().Single(t => t.Key == "test").Value);
});
}

private class TestHttpMetricsTagsFeature : IHttpMetricsTagsFeature
{
public ICollection<KeyValuePair<string, object>> Tags { get; } = new Collection<KeyValuePair<string, object>>();
}

[Fact]
public void DisposeContextDoesNotClearHttpContextIfDefaultHttpContextFactoryUsed()
{
Expand Down Expand Up @@ -204,7 +245,9 @@ public void IHttpActivityFeatureIsPopulated()
var initialActivity = Activity.Current;

// Create nested dummy Activity
using var _ = dummySource.StartActivity("DummyActivity");
using var dummyActivity = dummySource.StartActivity("DummyActivity");
Assert.NotNull(dummyActivity);
Assert.Equal(Activity.Current, dummyActivity);
Comment on lines +241 to +243
Copy link
Member Author

Choose a reason for hiding this comment

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

Attempting to narrow down the cause of this flaky test.


Assert.Same(initialActivity, activityFeature.Activity);
Assert.Null(activityFeature.Activity.ParentId);
Expand Down Expand Up @@ -247,7 +290,9 @@ public void IHttpActivityFeatureIsAssignedToIfItExists()
var initialActivity = Activity.Current;

// Create nested dummy Activity
using var _ = dummySource.StartActivity("DummyActivity");
using var dummyActivity = dummySource.StartActivity("DummyActivity");
Assert.NotNull(dummyActivity);
Assert.Equal(Activity.Current, dummyActivity);

Assert.Same(initialActivity, activityFeature.Activity);
Assert.Null(activityFeature.Activity.ParentId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,22 @@ internal async Task ExecuteAsync()
var connectionContext = _transportConnection;
var metricsConnectionDurationEnabled = _serviceContext.Metrics.IsConnectionDurationEnabled();
var startTimestamp = 0L;
ConnectionMetricsTagsFeature? metricsTagsFeature = null;
IConnectionMetricsTagsFeature? metricsTagsFeature = null;
Exception? unhandledException = null;

if (metricsConnectionDurationEnabled)
{
startTimestamp = Stopwatch.GetTimestamp();
if (connectionContext.Features.Get<IConnectionMetricsTagsFeature>() is IConnectionMetricsTagsFeature feature)
{
metricsTagsFeature = feature;
}
else
{
metricsTagsFeature = new ConnectionMetricsTagsFeature();
connectionContext.Features.Set(metricsTagsFeature);
}

metricsTagsFeature = new ConnectionMetricsTagsFeature();
connectionContext.Features.Set<IConnectionMetricsTagsFeature>(metricsTagsFeature);
startTimestamp = Stopwatch.GetTimestamp();
}

try
Expand Down Expand Up @@ -85,7 +92,7 @@ internal async Task ExecuteAsync()

Logger.ConnectionStop(connectionContext.ConnectionId);
KestrelEventSource.Log.ConnectionStop(connectionContext);
_serviceContext.Metrics.ConnectionStop(connectionContext, unhandledException, metricsTagsFeature?.TagsList, startTimestamp, currentTimestamp);
_serviceContext.Metrics.ConnectionStop(connectionContext, unhandledException, metricsTagsFeature?.Tags, startTimestamp, currentTimestamp);

// Dispose the transport connection, this needs to happen before removing it from the
// connection manager so that we only signal completion of this connection after the transport
Expand All @@ -98,7 +105,6 @@ internal async Task ExecuteAsync()

private sealed class ConnectionMetricsTagsFeature : IConnectionMetricsTagsFeature
{
public ICollection<KeyValuePair<string, object?>> Tags => TagsList;
public List<KeyValuePair<string, object?>> TagsList { get; } = new List<KeyValuePair<string, object?>>();
public ICollection<KeyValuePair<string, object?>> Tags { get; } = new List<KeyValuePair<string, object?>>();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ private void ConnectionStartCore(BaseConnectionContext connection)
_currentConnectionsCounter.Add(1, tags);
}

public void ConnectionStop(BaseConnectionContext connection, Exception? exception, List<KeyValuePair<string, object?>>? customTags, long startTimestamp, long currentTimestamp)
public void ConnectionStop(BaseConnectionContext connection, Exception? exception, ICollection<KeyValuePair<string, object?>>? customTags, long startTimestamp, long currentTimestamp)
{
if (_currentConnectionsCounter.Enabled || _connectionDuration.Enabled)
{
Expand All @@ -89,7 +89,7 @@ public void ConnectionStop(BaseConnectionContext connection, Exception? exceptio
}

[MethodImpl(MethodImplOptions.NoInlining)]
private void ConnectionStopCore(BaseConnectionContext connection, Exception? exception, List<KeyValuePair<string, object?>>? customTags, long startTimestamp, long currentTimestamp)
private void ConnectionStopCore(BaseConnectionContext connection, Exception? exception, ICollection<KeyValuePair<string, object?>>? customTags, long startTimestamp, long currentTimestamp)
{
var tags = new TagList();
InitializeConnectionTags(ref tags, connection);
Expand All @@ -105,16 +105,32 @@ private void ConnectionStopCore(BaseConnectionContext connection, Exception? exc
// Add custom tags for duration.
if (customTags != null)
{
for (var i = 0; i < customTags.Count; i++)
{
tags.Add(customTags[i]);
}
AddCustomTags(ref tags, customTags);
}

var duration = Stopwatch.GetElapsedTime(startTimestamp, currentTimestamp);
_connectionDuration.Record(duration.TotalSeconds, tags);
}

private static void AddCustomTags(ref TagList tags, ICollection<KeyValuePair<string, object?>> customTags)
{
// Skip allocating enumerator if tags collection is a list.
if (customTags is List<KeyValuePair<string, object?>> list)
{
for (var i = 0; i < list.Count; i++)
{
tags.Add(list[i]);
}
}
else
{
foreach (var tag in customTags)
{
tags.Add(tag);
}
}
}

public void ConnectionRejected(BaseConnectionContext connection)
{
if (_rejectedConnectionsCounter.Enabled)
Expand Down