diff --git a/src/Hosting/Hosting/src/Internal/HostingApplication.cs b/src/Hosting/Hosting/src/Internal/HostingApplication.cs index b880a7d3106b..b94f2e9b8b89 100644 --- a/src/Hosting/Hosting/src/Internal/HostingApplication.cs +++ b/src/Hosting/Hosting/src/Internal/HostingApplication.cs @@ -129,7 +129,10 @@ public Activity? Activity { if (HttpActivityFeature is null) { - HttpActivityFeature = new ActivityFeature(value!); + if (value != null) + { + HttpActivityFeature = new HttpActivityFeature(value); + } } else { @@ -143,7 +146,7 @@ public Activity? Activity internal bool HasDiagnosticListener { get; set; } public bool EventLogOrMetricsEnabled { get; set; } - internal IHttpActivityFeature? HttpActivityFeature; + internal HttpActivityFeature? HttpActivityFeature; internal HttpMetricsTagsFeature? MetricsTagsFeature; public void Reset() diff --git a/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs b/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs index fb24de39de1c..4d4c4a8a4a20 100644 --- a/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs +++ b/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs @@ -55,15 +55,8 @@ public void BeginRequest(HttpContext httpContext, HostingApplication.Context con if (_eventSource.IsEnabled() || _metrics.IsEnabled()) { context.EventLogOrMetricsEnabled = true; - if (httpContext.Features.Get() is HttpMetricsTagsFeature feature) - { - context.MetricsTagsFeature = feature; - } - else - { - context.MetricsTagsFeature ??= new HttpMetricsTagsFeature(); - httpContext.Features.Set(context.MetricsTagsFeature); - } + context.MetricsTagsFeature ??= new HttpMetricsTagsFeature(); + httpContext.Features.Set(context.MetricsTagsFeature); startTimestamp = Stopwatch.GetTimestamp(); @@ -80,16 +73,9 @@ public void BeginRequest(HttpContext httpContext, HostingApplication.Context con context.Activity = StartActivity(httpContext, loggingEnabled, diagnosticListenerActivityCreationEnabled, out var hasDiagnosticListener); context.HasDiagnosticListener = hasDiagnosticListener; - if (context.Activity is Activity activity) + if (context.Activity != null) { - if (httpContext.Features.Get() is IHttpActivityFeature feature) - { - feature.Activity = activity; - } - else - { - httpContext.Features.Set(context.HttpActivityFeature); - } + httpContext.Features.Set(context.HttpActivityFeature); } } diff --git a/src/Hosting/Hosting/src/Internal/ActivityFeature.cs b/src/Hosting/Hosting/src/Internal/HttpActivityFeature.cs similarity index 79% rename from src/Hosting/Hosting/src/Internal/ActivityFeature.cs rename to src/Hosting/Hosting/src/Internal/HttpActivityFeature.cs index f89495c78bc9..d7fd773fa8fc 100644 --- a/src/Hosting/Hosting/src/Internal/ActivityFeature.cs +++ b/src/Hosting/Hosting/src/Internal/HttpActivityFeature.cs @@ -9,9 +9,9 @@ namespace Microsoft.AspNetCore.Hosting; /// /// Default implementation for . /// -internal sealed class ActivityFeature : IHttpActivityFeature +internal sealed class HttpActivityFeature : IHttpActivityFeature { - internal ActivityFeature(Activity activity) + internal HttpActivityFeature(Activity activity) { Activity = activity; } diff --git a/src/Hosting/Hosting/test/HostingApplicationTests.cs b/src/Hosting/Hosting/test/HostingApplicationTests.cs index a0154713d137..d3e9e2623b09 100644 --- a/src/Hosting/Hosting/test/HostingApplicationTests.cs +++ b/src/Hosting/Hosting/test/HostingApplicationTests.cs @@ -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; @@ -108,6 +109,39 @@ static void AssertRequestDuration(Measurement measurement, string protoc } } + [Fact] + public void IHttpMetricsTagsFeatureNotUsedFromFeatureCollection() + { + // 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(meterRegistry, HostingMetrics.MeterName, "request-duration"); + using var currentRequestsRecorder = new InstrumentRecorder(meterRegistry, HostingMetrics.MeterName, "current-requests"); + + // Act/Assert + Assert.Equal(HostingMetrics.MeterName, meter.Name); + Assert.Null(meter.Version); + + // This feature will be overidden by hosting. Hosting is the owner of the feature and is resposible for setting it. + var overridenFeature = new TestHttpMetricsTagsFeature(); + httpContext.Features.Set(overridenFeature); + + var context = hostingApplication.CreateContext(httpContext.Features); + var contextFeature = httpContext.Features.Get(); + + Assert.NotNull(contextFeature); + Assert.NotEqual(overridenFeature, contextFeature); + } + + private sealed class TestHttpMetricsTagsFeature : IHttpMetricsTagsFeature + { + public ICollection> Tags { get; } = new Collection>(); + } + [Fact] public void DisposeContextDoesNotClearHttpContextIfDefaultHttpContextFactoryUsed() { @@ -204,7 +238,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); Assert.Same(initialActivity, activityFeature.Activity); Assert.Null(activityFeature.Activity.ParentId); @@ -221,8 +257,7 @@ private class TestHttpActivityFeature : IHttpActivityFeature } [Fact] - [QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/38736")] - public void IHttpActivityFeatureIsAssignedToIfItExists() + public void IHttpActivityFeatureNotUsedFromFeatureCollection() { var testSource = new ActivitySource(Path.GetRandomFileName()); var dummySource = new ActivitySource(Path.GetRandomFileName()); @@ -236,26 +271,19 @@ public void IHttpActivityFeatureIsAssignedToIfItExists() var hostingApplication = CreateApplication(activitySource: testSource); var httpContext = new DefaultHttpContext(); - httpContext.Features.Set(new TestHttpActivityFeature()); - var context = hostingApplication.CreateContext(httpContext.Features); - var activityFeature = context.HttpContext.Features.Get(); - Assert.NotNull(activityFeature); - Assert.IsType(activityFeature); - Assert.NotNull(activityFeature.Activity); - Assert.Equal(HostingApplicationDiagnostics.ActivityName, activityFeature.Activity.DisplayName); - var initialActivity = Activity.Current; + // This feature will be overidden by hosting. Hosting is the owner of the feature and is resposible for setting it. + var overridenFeature = new TestHttpActivityFeature(); + httpContext.Features.Set(overridenFeature); - // Create nested dummy Activity - using var _ = dummySource.StartActivity("DummyActivity"); + var context = hostingApplication.CreateContext(httpContext.Features); - Assert.Same(initialActivity, activityFeature.Activity); - Assert.Null(activityFeature.Activity.ParentId); - Assert.Equal(activityFeature.Activity.Id, Activity.Current.ParentId); - Assert.NotEqual(Activity.Current, activityFeature.Activity); + var contextFeature = context.HttpContext.Features.Get(); + Assert.NotNull(contextFeature); + Assert.NotNull(contextFeature.Activity); + Assert.Equal(HostingApplicationDiagnostics.ActivityName, contextFeature.Activity.DisplayName); - // Act/Assert - hostingApplication.DisposeContext(context, null); + Assert.NotEqual(overridenFeature, contextFeature); } [Fact] diff --git a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelConnectionOfT.cs b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelConnectionOfT.cs index 22c84ae704ce..2cb095ce3258 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelConnectionOfT.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelConnectionOfT.cs @@ -45,10 +45,10 @@ internal async Task ExecuteAsync() if (metricsConnectionDurationEnabled) { - startTimestamp = Stopwatch.GetTimestamp(); - metricsTagsFeature = new ConnectionMetricsTagsFeature(); connectionContext.Features.Set(metricsTagsFeature); + + startTimestamp = Stopwatch.GetTimestamp(); } try @@ -99,6 +99,7 @@ internal async Task ExecuteAsync() private sealed class ConnectionMetricsTagsFeature : IConnectionMetricsTagsFeature { public ICollection> Tags => TagsList; + public List> TagsList { get; } = new List>(); } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/KestrelMetricsTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/KestrelMetricsTests.cs index 656b854bdc1f..b2b29966c1a9 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/KestrelMetricsTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/KestrelMetricsTests.cs @@ -88,6 +88,86 @@ await connection.ReceiveEnd( Assert.Collection(queuedConnections.GetMeasurements(), m => AssertCount(m, 1, "127.0.0.1:0"), m => AssertCount(m, -1, "127.0.0.1:0")); } + [Fact] + public async Task Http1Connection_IHttpConnectionTagsFeatureIgnoreFeatureSetOnTransport() + { + var sync = new SyncPoint(); + ConnectionContext currentConnectionContext = null; + + var listenOptions = new ListenOptions(new IPEndPoint(IPAddress.Loopback, 0)); + listenOptions.Use(next => + { + return async connectionContext => + { + currentConnectionContext = connectionContext; + + connectionContext.Features.Get().Tags.Add(new KeyValuePair("custom", "value!")); + + // Wait for the test to verify the connection has started. + await sync.WaitToContinue(); + + await next(connectionContext); + }; + }); + + var testMeterFactory = new TestMeterFactory(); + using var connectionDuration = new InstrumentRecorder(new TestMeterRegistry(testMeterFactory.Meters), "Microsoft.AspNetCore.Server.Kestrel", "connection-duration"); + using var currentConnections = new InstrumentRecorder(new TestMeterRegistry(testMeterFactory.Meters), "Microsoft.AspNetCore.Server.Kestrel", "current-connections"); + using var queuedConnections = new InstrumentRecorder(new TestMeterRegistry(testMeterFactory.Meters), "Microsoft.AspNetCore.Server.Kestrel", "queued-connections"); + + var serviceContext = new TestServiceContext(LoggerFactory, metrics: new KestrelMetrics(testMeterFactory)); + + var sendString = "POST / HTTP/1.0\r\nContent-Length: 12\r\n\r\nHello World?"; + + await using var server = new TestServer(EchoApp, serviceContext, listenOptions); + + // This feature will be overidden by Kestrel. Kestrel is the owner of the feature and is resposible for setting it. + var overridenFeature = new TestConnectionMetricsTagsFeature(); + overridenFeature.Tags.Add(new KeyValuePair("test", "Value!")); + + using (var connection = server.CreateConnection(featuresAction: features => + { + features.Set(overridenFeature); + })) + { + await connection.Send(sendString); + + // Wait for connection to start on the server. + await sync.WaitForSyncPoint(); + + Assert.NotEqual(overridenFeature, currentConnectionContext.Features.Get()); + + Assert.Empty(connectionDuration.GetMeasurements()); + Assert.Collection(currentConnections.GetMeasurements(), m => AssertCount(m, 1, "127.0.0.1:0")); + + // Signal that connection can continue. + sync.Continue(); + + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Connection: close", + $"Date: {serviceContext.DateHeaderValue}", + "", + "Hello World?"); + + await connection.WaitForConnectionClose(); + } + + Assert.Collection(connectionDuration.GetMeasurements(), m => + { + AssertDuration(m, "127.0.0.1:0"); + Assert.Equal("value!", (string)m.Tags.ToArray().Single(t => t.Key == "custom").Value); + Assert.Empty(m.Tags.ToArray().Where(t => t.Key == "test")); + }); + Assert.Collection(currentConnections.GetMeasurements(), m => AssertCount(m, 1, "127.0.0.1:0"), m => AssertCount(m, -1, "127.0.0.1:0")); + Assert.Collection(queuedConnections.GetMeasurements(), m => AssertCount(m, 1, "127.0.0.1:0"), m => AssertCount(m, -1, "127.0.0.1:0")); + } + + private sealed class TestConnectionMetricsTagsFeature : IConnectionMetricsTagsFeature + { + public ICollection> Tags { get; } = new List>(); + } + [Fact] public async Task Http1Connection_Error() { diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/TestTransport/TestServer.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/TestTransport/TestServer.cs index ee9e1efa529d..976cb389131b 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/TestTransport/TestServer.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/TestTransport/TestServer.cs @@ -16,6 +16,7 @@ using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Hosting.Server; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Server.Kestrel.Core; using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.DependencyInjection; @@ -109,9 +110,11 @@ public TestServer(RequestDelegate app, TestServiceContext context, Action featuresAction = null) { var transportConnection = new InMemoryTransportConnection(_memoryPool, Context.Log, Context.Scheduler); + featuresAction?.Invoke(transportConnection.Features); + _transportFactory.AddConnection(transportConnection); return new InMemoryConnection(transportConnection, encoding); }