Skip to content

Commit 31d335b

Browse files
JamesNKJamesNK
authored
Diaganostics and metrics clean up (dotnet#47679)
1 parent e533799 commit 31d335b

File tree

7 files changed

+145
-44
lines changed

7 files changed

+145
-44
lines changed

‎src/Hosting/Hosting/src/Internal/HostingApplication.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,10 @@ public Activity? Activity
129129
{
130130
if (HttpActivityFeature is null)
131131
{
132-
HttpActivityFeature = new ActivityFeature(value!);
132+
if (value != null)
133+
{
134+
HttpActivityFeature = new HttpActivityFeature(value);
135+
}
133136
}
134137
else
135138
{
@@ -143,7 +146,7 @@ public Activity? Activity
143146
internal bool HasDiagnosticListener { get; set; }
144147
public bool EventLogOrMetricsEnabled { get; set; }
145148

146-
internal IHttpActivityFeature? HttpActivityFeature;
149+
internal HttpActivityFeature? HttpActivityFeature;
147150
internal HttpMetricsTagsFeature? MetricsTagsFeature;
148151

149152
public void Reset()

‎src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs

Lines changed: 4 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -55,15 +55,8 @@ public void BeginRequest(HttpContext httpContext, HostingApplication.Context con
5555
if (_eventSource.IsEnabled() || _metrics.IsEnabled())
5656
{
5757
context.EventLogOrMetricsEnabled = true;
58-
if (httpContext.Features.Get<IHttpMetricsTagsFeature>() is HttpMetricsTagsFeature feature)
59-
{
60-
context.MetricsTagsFeature = feature;
61-
}
62-
else
63-
{
64-
context.MetricsTagsFeature ??= new HttpMetricsTagsFeature();
65-
httpContext.Features.Set<IHttpMetricsTagsFeature>(context.MetricsTagsFeature);
66-
}
58+
context.MetricsTagsFeature ??= new HttpMetricsTagsFeature();
59+
httpContext.Features.Set<IHttpMetricsTagsFeature>(context.MetricsTagsFeature);
6760

6861
startTimestamp = Stopwatch.GetTimestamp();
6962

@@ -80,16 +73,9 @@ public void BeginRequest(HttpContext httpContext, HostingApplication.Context con
8073
context.Activity = StartActivity(httpContext, loggingEnabled, diagnosticListenerActivityCreationEnabled, out var hasDiagnosticListener);
8174
context.HasDiagnosticListener = hasDiagnosticListener;
8275

83-
if (context.Activity is Activity activity)
76+
if (context.Activity != null)
8477
{
85-
if (httpContext.Features.Get<IHttpActivityFeature>() is IHttpActivityFeature feature)
86-
{
87-
feature.Activity = activity;
88-
}
89-
else
90-
{
91-
httpContext.Features.Set(context.HttpActivityFeature);
92-
}
78+
httpContext.Features.Set<IHttpActivityFeature>(context.HttpActivityFeature);
9379
}
9480
}
9581

‎src/Hosting/Hosting/src/Internal/ActivityFeature.cs renamed to ‎src/Hosting/Hosting/src/Internal/HttpActivityFeature.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@ namespace Microsoft.AspNetCore.Hosting;
99
/// <summary>
1010
/// Default implementation for <see cref="IHttpActivityFeature"/>.
1111
/// </summary>
12-
internal sealed class ActivityFeature : IHttpActivityFeature
12+
internal sealed class HttpActivityFeature : IHttpActivityFeature
1313
{
14-
internal ActivityFeature(Activity activity)
14+
internal HttpActivityFeature(Activity activity)
1515
{
1616
Activity = activity;
1717
}

‎src/Hosting/Hosting/test/HostingApplicationTests.cs

Lines changed: 47 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.Collections;
5+
using System.Collections.ObjectModel;
56
using System.Diagnostics;
67
using System.Diagnostics.Metrics;
78
using Microsoft.AspNetCore.Hosting.Fakes;
@@ -108,6 +109,39 @@ static void AssertRequestDuration(Measurement<double> measurement, string protoc
108109
}
109110
}
110111

112+
[Fact]
113+
public void IHttpMetricsTagsFeatureNotUsedFromFeatureCollection()
114+
{
115+
// Arrange
116+
var meterFactory = new TestMeterFactory();
117+
var meterRegistry = new TestMeterRegistry(meterFactory.Meters);
118+
var hostingApplication = CreateApplication(meterFactory: meterFactory);
119+
var httpContext = new DefaultHttpContext();
120+
var meter = meterFactory.Meters.Single();
121+
122+
using var requestDurationRecorder = new InstrumentRecorder<double>(meterRegistry, HostingMetrics.MeterName, "request-duration");
123+
using var currentRequestsRecorder = new InstrumentRecorder<long>(meterRegistry, HostingMetrics.MeterName, "current-requests");
124+
125+
// Act/Assert
126+
Assert.Equal(HostingMetrics.MeterName, meter.Name);
127+
Assert.Null(meter.Version);
128+
129+
// This feature will be overidden by hosting. Hosting is the owner of the feature and is resposible for setting it.
130+
var overridenFeature = new TestHttpMetricsTagsFeature();
131+
httpContext.Features.Set<IHttpMetricsTagsFeature>(overridenFeature);
132+
133+
var context = hostingApplication.CreateContext(httpContext.Features);
134+
var contextFeature = httpContext.Features.Get<IHttpMetricsTagsFeature>();
135+
136+
Assert.NotNull(contextFeature);
137+
Assert.NotEqual(overridenFeature, contextFeature);
138+
}
139+
140+
private sealed class TestHttpMetricsTagsFeature : IHttpMetricsTagsFeature
141+
{
142+
public ICollection<KeyValuePair<string, object>> Tags { get; } = new Collection<KeyValuePair<string, object>>();
143+
}
144+
111145
[Fact]
112146
public void DisposeContextDoesNotClearHttpContextIfDefaultHttpContextFactoryUsed()
113147
{
@@ -204,7 +238,9 @@ public void IHttpActivityFeatureIsPopulated()
204238
var initialActivity = Activity.Current;
205239

206240
// Create nested dummy Activity
207-
using var _ = dummySource.StartActivity("DummyActivity");
241+
using var dummyActivity = dummySource.StartActivity("DummyActivity");
242+
Assert.NotNull(dummyActivity);
243+
Assert.Equal(Activity.Current, dummyActivity);
208244

209245
Assert.Same(initialActivity, activityFeature.Activity);
210246
Assert.Null(activityFeature.Activity.ParentId);
@@ -221,8 +257,7 @@ private class TestHttpActivityFeature : IHttpActivityFeature
221257
}
222258

223259
[Fact]
224-
[QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/38736")]
225-
public void IHttpActivityFeatureIsAssignedToIfItExists()
260+
public void IHttpActivityFeatureNotUsedFromFeatureCollection()
226261
{
227262
var testSource = new ActivitySource(Path.GetRandomFileName());
228263
var dummySource = new ActivitySource(Path.GetRandomFileName());
@@ -236,26 +271,19 @@ public void IHttpActivityFeatureIsAssignedToIfItExists()
236271

237272
var hostingApplication = CreateApplication(activitySource: testSource);
238273
var httpContext = new DefaultHttpContext();
239-
httpContext.Features.Set<IHttpActivityFeature>(new TestHttpActivityFeature());
240-
var context = hostingApplication.CreateContext(httpContext.Features);
241274

242-
var activityFeature = context.HttpContext.Features.Get<IHttpActivityFeature>();
243-
Assert.NotNull(activityFeature);
244-
Assert.IsType<TestHttpActivityFeature>(activityFeature);
245-
Assert.NotNull(activityFeature.Activity);
246-
Assert.Equal(HostingApplicationDiagnostics.ActivityName, activityFeature.Activity.DisplayName);
247-
var initialActivity = Activity.Current;
275+
// This feature will be overidden by hosting. Hosting is the owner of the feature and is resposible for setting it.
276+
var overridenFeature = new TestHttpActivityFeature();
277+
httpContext.Features.Set<IHttpActivityFeature>(overridenFeature);
248278

249-
// Create nested dummy Activity
250-
using var _ = dummySource.StartActivity("DummyActivity");
279+
var context = hostingApplication.CreateContext(httpContext.Features);
251280

252-
Assert.Same(initialActivity, activityFeature.Activity);
253-
Assert.Null(activityFeature.Activity.ParentId);
254-
Assert.Equal(activityFeature.Activity.Id, Activity.Current.ParentId);
255-
Assert.NotEqual(Activity.Current, activityFeature.Activity);
281+
var contextFeature = context.HttpContext.Features.Get<IHttpActivityFeature>();
282+
Assert.NotNull(contextFeature);
283+
Assert.NotNull(contextFeature.Activity);
284+
Assert.Equal(HostingApplicationDiagnostics.ActivityName, contextFeature.Activity.DisplayName);
256285

257-
// Act/Assert
258-
hostingApplication.DisposeContext(context, null);
286+
Assert.NotEqual(overridenFeature, contextFeature);
259287
}
260288

261289
[Fact]

‎src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelConnectionOfT.cs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,10 @@ internal async Task ExecuteAsync()
4545

4646
if (metricsConnectionDurationEnabled)
4747
{
48-
startTimestamp = Stopwatch.GetTimestamp();
49-
5048
metricsTagsFeature = new ConnectionMetricsTagsFeature();
5149
connectionContext.Features.Set<IConnectionMetricsTagsFeature>(metricsTagsFeature);
50+
51+
startTimestamp = Stopwatch.GetTimestamp();
5252
}
5353

5454
try
@@ -99,6 +99,7 @@ internal async Task ExecuteAsync()
9999
private sealed class ConnectionMetricsTagsFeature : IConnectionMetricsTagsFeature
100100
{
101101
public ICollection<KeyValuePair<string, object?>> Tags => TagsList;
102+
102103
public List<KeyValuePair<string, object?>> TagsList { get; } = new List<KeyValuePair<string, object?>>();
103104
}
104105
}

‎src/Servers/Kestrel/test/InMemory.FunctionalTests/KestrelMetricsTests.cs

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,86 @@ await connection.ReceiveEnd(
8888
Assert.Collection(queuedConnections.GetMeasurements(), m => AssertCount(m, 1, "127.0.0.1:0"), m => AssertCount(m, -1, "127.0.0.1:0"));
8989
}
9090

91+
[Fact]
92+
public async Task Http1Connection_IHttpConnectionTagsFeatureIgnoreFeatureSetOnTransport()
93+
{
94+
var sync = new SyncPoint();
95+
ConnectionContext currentConnectionContext = null;
96+
97+
var listenOptions = new ListenOptions(new IPEndPoint(IPAddress.Loopback, 0));
98+
listenOptions.Use(next =>
99+
{
100+
return async connectionContext =>
101+
{
102+
currentConnectionContext = connectionContext;
103+
104+
connectionContext.Features.Get<IConnectionMetricsTagsFeature>().Tags.Add(new KeyValuePair<string, object>("custom", "value!"));
105+
106+
// Wait for the test to verify the connection has started.
107+
await sync.WaitToContinue();
108+
109+
await next(connectionContext);
110+
};
111+
});
112+
113+
var testMeterFactory = new TestMeterFactory();
114+
using var connectionDuration = new InstrumentRecorder<double>(new TestMeterRegistry(testMeterFactory.Meters), "Microsoft.AspNetCore.Server.Kestrel", "connection-duration");
115+
using var currentConnections = new InstrumentRecorder<long>(new TestMeterRegistry(testMeterFactory.Meters), "Microsoft.AspNetCore.Server.Kestrel", "current-connections");
116+
using var queuedConnections = new InstrumentRecorder<long>(new TestMeterRegistry(testMeterFactory.Meters), "Microsoft.AspNetCore.Server.Kestrel", "queued-connections");
117+
118+
var serviceContext = new TestServiceContext(LoggerFactory, metrics: new KestrelMetrics(testMeterFactory));
119+
120+
var sendString = "POST / HTTP/1.0\r\nContent-Length: 12\r\n\r\nHello World?";
121+
122+
await using var server = new TestServer(EchoApp, serviceContext, listenOptions);
123+
124+
// This feature will be overidden by Kestrel. Kestrel is the owner of the feature and is resposible for setting it.
125+
var overridenFeature = new TestConnectionMetricsTagsFeature();
126+
overridenFeature.Tags.Add(new KeyValuePair<string, object>("test", "Value!"));
127+
128+
using (var connection = server.CreateConnection(featuresAction: features =>
129+
{
130+
features.Set<IConnectionMetricsTagsFeature>(overridenFeature);
131+
}))
132+
{
133+
await connection.Send(sendString);
134+
135+
// Wait for connection to start on the server.
136+
await sync.WaitForSyncPoint();
137+
138+
Assert.NotEqual(overridenFeature, currentConnectionContext.Features.Get<IConnectionMetricsTagsFeature>());
139+
140+
Assert.Empty(connectionDuration.GetMeasurements());
141+
Assert.Collection(currentConnections.GetMeasurements(), m => AssertCount(m, 1, "127.0.0.1:0"));
142+
143+
// Signal that connection can continue.
144+
sync.Continue();
145+
146+
await connection.ReceiveEnd(
147+
"HTTP/1.1 200 OK",
148+
"Connection: close",
149+
$"Date: {serviceContext.DateHeaderValue}",
150+
"",
151+
"Hello World?");
152+
153+
await connection.WaitForConnectionClose();
154+
}
155+
156+
Assert.Collection(connectionDuration.GetMeasurements(), m =>
157+
{
158+
AssertDuration(m, "127.0.0.1:0");
159+
Assert.Equal("value!", (string)m.Tags.ToArray().Single(t => t.Key == "custom").Value);
160+
Assert.Empty(m.Tags.ToArray().Where(t => t.Key == "test"));
161+
});
162+
Assert.Collection(currentConnections.GetMeasurements(), m => AssertCount(m, 1, "127.0.0.1:0"), m => AssertCount(m, -1, "127.0.0.1:0"));
163+
Assert.Collection(queuedConnections.GetMeasurements(), m => AssertCount(m, 1, "127.0.0.1:0"), m => AssertCount(m, -1, "127.0.0.1:0"));
164+
}
165+
166+
private sealed class TestConnectionMetricsTagsFeature : IConnectionMetricsTagsFeature
167+
{
168+
public ICollection<KeyValuePair<string, object>> Tags { get; } = new List<KeyValuePair<string, object>>();
169+
}
170+
91171
[Fact]
92172
public async Task Http1Connection_Error()
93173
{

‎src/Servers/Kestrel/test/InMemory.FunctionalTests/TestTransport/TestServer.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
using Microsoft.AspNetCore.Hosting;
1717
using Microsoft.AspNetCore.Hosting.Server;
1818
using Microsoft.AspNetCore.Http;
19+
using Microsoft.AspNetCore.Http.Features;
1920
using Microsoft.AspNetCore.Server.Kestrel.Core;
2021
using Microsoft.AspNetCore.Testing;
2122
using Microsoft.Extensions.DependencyInjection;
@@ -112,9 +113,11 @@ public TestServer(RequestDelegate app, TestServiceContext context, Action<Kestre
112113

113114
public InMemoryHttpClientSlim HttpClientSlim { get; }
114115

115-
public InMemoryConnection CreateConnection(Encoding encoding = null)
116+
public InMemoryConnection CreateConnection(Encoding encoding = null, Action<IFeatureCollection> featuresAction = null)
116117
{
117118
var transportConnection = new InMemoryTransportConnection(_memoryPool, Context.Log, Context.Scheduler);
119+
featuresAction?.Invoke(transportConnection.Features);
120+
118121
_transportFactory.AddConnection(transportConnection);
119122
return new InMemoryConnection(transportConnection, encoding);
120123
}

0 commit comments

Comments
 (0)