Diaganostics and metrics clean up (dotnet#47679)

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()

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<IHttpMetricsTagsFeature>() is HttpMetricsTagsFeature feature)
-            {
-                context.MetricsTagsFeature = feature;
-            }
-            else
-            {
-                context.MetricsTagsFeature ??= new HttpMetricsTagsFeature();
-                httpContext.Features.Set<IHttpMetricsTagsFeature>(context.MetricsTagsFeature);
-            }
+            context.MetricsTagsFeature ??= new HttpMetricsTagsFeature();
+            httpContext.Features.Set<IHttpMetricsTagsFeature>(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<IHttpActivityFeature>() is IHttpActivityFeature feature)
-                {
-                    feature.Activity = activity;
-                }
-                else
-                {
-                    httpContext.Features.Set(context.HttpActivityFeature);
-                }
+                httpContext.Features.Set<IHttpActivityFeature>(context.HttpActivityFeature);
             }
         }
 

src/Hosting/Hosting/src/Internal/ActivityFeature.cs → src/Hosting/Hosting/src/Internal/HttpActivityFeature.cs

@@ -9,9 +9,9 @@ namespace Microsoft.AspNetCore.Hosting;
 /// <summary>
 /// Default implementation for <see cref="IHttpActivityFeature"/>.
 /// </summary>
-internal sealed class ActivityFeature : IHttpActivityFeature
+internal sealed class HttpActivityFeature : IHttpActivityFeature
 {
-    internal ActivityFeature(Activity activity)
+    internal HttpActivityFeature(Activity activity)
     {
         Activity = activity;
     }

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<double> 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<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);
+
+        // 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<IHttpMetricsTagsFeature>(overridenFeature);
+
+        var context = hostingApplication.CreateContext(httpContext.Features);
+        var contextFeature = httpContext.Features.Get<IHttpMetricsTagsFeature>();
+
+        Assert.NotNull(contextFeature);
+        Assert.NotEqual(overridenFeature, contextFeature);
+    }
+
+    private sealed class TestHttpMetricsTagsFeature : IHttpMetricsTagsFeature
+    {
+        public ICollection<KeyValuePair<string, object>> Tags { get; } = new Collection<KeyValuePair<string, object>>();
+    }
+
     [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<IHttpActivityFeature>(new TestHttpActivityFeature());
-        var context = hostingApplication.CreateContext(httpContext.Features);
 
-        var activityFeature = context.HttpContext.Features.Get<IHttpActivityFeature>();
-        Assert.NotNull(activityFeature);
-        Assert.IsType<TestHttpActivityFeature>(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<IHttpActivityFeature>(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<IHttpActivityFeature>();
+        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]

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<IConnectionMetricsTagsFeature>(metricsTagsFeature);
+
+            startTimestamp = Stopwatch.GetTimestamp();
         }
 
         try
@@ -99,6 +99,7 @@ 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?>>();
     }
 }

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<IConnectionMetricsTagsFeature>().Tags.Add(new KeyValuePair<string, object>("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<double>(new TestMeterRegistry(testMeterFactory.Meters), "Microsoft.AspNetCore.Server.Kestrel", "connection-duration");
+        using var currentConnections = new InstrumentRecorder<long>(new TestMeterRegistry(testMeterFactory.Meters), "Microsoft.AspNetCore.Server.Kestrel", "current-connections");
+        using var queuedConnections = new InstrumentRecorder<long>(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<string, object>("test", "Value!"));
+
+        using (var connection = server.CreateConnection(featuresAction: features =>
+        {
+            features.Set<IConnectionMetricsTagsFeature>(overridenFeature);
+        }))
+        {
+            await connection.Send(sendString);
+
+            // Wait for connection to start on the server.
+            await sync.WaitForSyncPoint();
+
+            Assert.NotEqual(overridenFeature, currentConnectionContext.Features.Get<IConnectionMetricsTagsFeature>());
+
+            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<KeyValuePair<string, object>> Tags { get; } = new List<KeyValuePair<string, object>>();
+    }
+
     [Fact]
     public async Task Http1Connection_Error()
     {

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;
@@ -112,9 +113,11 @@ public TestServer(RequestDelegate app, TestServiceContext context, Action<Kestre
 
     public InMemoryHttpClientSlim HttpClientSlim { get; }
 
-    public InMemoryConnection CreateConnection(Encoding encoding = null)
+    public InMemoryConnection CreateConnection(Encoding encoding = null, Action<IFeatureCollection> featuresAction = null)
     {
         var transportConnection = new InMemoryTransportConnection(_memoryPool, Context.Log, Context.Scheduler);
+        featuresAction?.Invoke(transportConnection.Features);
+
         _transportFactory.AddConnection(transportConnection);
         return new InMemoryConnection(transportConnection, encoding);
     }