[release/10.0-rc1] Empty "http.route" tags should be set to "/" (#63336)

* HostingMetrics: empty "http.route" tags shouldn't be present

* HostingMetrics: empty "http.route" tags should be set to "/"

These changes have been decided in #62431

* Do not change behavior if route is null, only if it's empty

* Add comment

* Rollback visual studio autoformatting on tests

* Update routing metric to be consistent

* Clean up

---------

Co-authored-by: Elanis <[email protected]>
Co-authored-by: Sébastien Ros <[email protected]>
Co-authored-by: James Newton-King <[email protected]>

Author: 4 people

src/Hosting/Hosting/src/Internal/HostingMetrics.cs

@@ -4,6 +4,7 @@
 using System.Diagnostics;
 using System.Diagnostics.Metrics;
 using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Shared;
 
 namespace Microsoft.AspNetCore.Hosting;
 
@@ -66,7 +67,7 @@ public void RequestEnd(string protocol, string scheme, string method, string? ro
             tags.Add("http.response.status_code", HostingTelemetryHelpers.GetBoxedStatusCode(statusCode));
             if (route != null)
             {
-                tags.Add("http.route", route);
+                tags.Add("http.route", RouteDiagnosticsHelpers.ResolveHttpRoute(route));
             }
 
             // Add before some built in tags so custom tags are prioritized when dealing with duplicates.

src/Hosting/Hosting/src/Microsoft.AspNetCore.Hosting.csproj

@@ -19,6 +19,7 @@
     <Compile Include="$(SharedSourceRoot)Metrics\MetricsExtensions.cs" />
     <Compile Include="$(SharedSourceRoot)Metrics\MetricsConstants.cs" />
     <Compile Include="$(SharedSourceRoot)Diagnostics\ActivityCreator.cs" />
+    <Compile Include="$(SharedSourceRoot)Diagnostics\RouteDiagnosticsHelpers.cs" />
   </ItemGroup>
 
   <ItemGroup>

src/Hosting/Hosting/test/HostingApplicationDiagnosticsTests.cs

@@ -352,6 +352,71 @@ public void Metrics_Route_RouteTagReported()
             });
     }
 
+    private sealed class EmptyRouteDiagnosticsMetadata : IRouteDiagnosticsMetadata
+    {
+        public string Route { get; } = "";
+    }
+
+    [Fact]
+    public void Metrics_Route_RouteTagIsRootWhenEmpty()
+    {
+        // Arrange
+        var hostingEventSource = new HostingEventSource(Guid.NewGuid().ToString());
+
+        var testMeterFactory = new TestMeterFactory();
+        using var activeRequestsCollector = new MetricCollector<long>(testMeterFactory, HostingMetrics.MeterName, "http.server.active_requests");
+        using var requestDurationCollector = new MetricCollector<double>(testMeterFactory, HostingMetrics.MeterName, "http.server.request.duration");
+
+        // Act
+        var hostingApplication = CreateApplication(out var features, eventSource: hostingEventSource, meterFactory: testMeterFactory, configure: c =>
+        {
+            c.Request.Protocol = "1.1";
+            c.Request.Scheme = "http";
+            c.Request.Method = "POST";
+            c.Request.Host = new HostString("localhost");
+            c.Request.Path = "";
+            c.Request.ContentType = "text/plain";
+            c.Request.ContentLength = 1024;
+        });
+        var context = hostingApplication.CreateContext(features);
+
+        Assert.Collection(activeRequestsCollector.GetMeasurementSnapshot(),
+            m =>
+            {
+                Assert.Equal(1, m.Value);
+                Assert.Equal("http", m.Tags["url.scheme"]);
+                Assert.Equal("POST", m.Tags["http.request.method"]);
+            });
+
+        context.HttpContext.SetEndpoint(new Endpoint(
+            c => Task.CompletedTask,
+            new EndpointMetadataCollection(new EmptyRouteDiagnosticsMetadata()),
+            "Test empty endpoint"));
+
+        hostingApplication.DisposeContext(context, null);
+
+        // Assert
+        Assert.Collection(activeRequestsCollector.GetMeasurementSnapshot(),
+            m =>
+            {
+                Assert.Equal(1, m.Value);
+                Assert.Equal("http", m.Tags["url.scheme"]);
+                Assert.Equal("POST", m.Tags["http.request.method"]);
+            },
+            m =>
+            {
+                Assert.Equal(-1, m.Value);
+                Assert.Equal("http", m.Tags["url.scheme"]);
+                Assert.Equal("POST", m.Tags["http.request.method"]);
+            });
+        Assert.Collection(requestDurationCollector.GetMeasurementSnapshot(),
+            m =>
+            {
+                Assert.True(m.Value > 0);
+                Assert.Equal("/", m.Tags["http.route"]);
+            });
+    }
+
     [Fact]
     public void Metrics_DisableHttpMetricsWithMetadata_NoMetrics()
     {

src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj

@@ -38,6 +38,7 @@
     <Compile Include="$(SharedSourceRoot)AntiforgeryMetadata.cs" LinkBase="Shared" />
     <Compile Include="$(SharedSourceRoot)HttpExtensions.cs" LinkBase="Shared" />
     <Compile Include="$(SharedSourceRoot)ContentTypeConstants.cs" LinkBase="Shared" />
+    <Compile Include="$(SharedSourceRoot)Diagnostics\RouteDiagnosticsHelpers.cs" LinkBase="Shared" />
   </ItemGroup>
 
   <ItemGroup>

src/Http/Routing/src/RoutingMetrics.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Diagnostics.Metrics;
+using Microsoft.AspNetCore.Shared;
 
 namespace Microsoft.AspNetCore.Routing;
 
@@ -31,7 +32,7 @@ public RoutingMetrics(IMeterFactory meterFactory)
     public void MatchSuccess(string route, bool isFallback)
     {
         _matchAttemptsCounter.Add(1,
-            new KeyValuePair<string, object?>("http.route", route),
+            new KeyValuePair<string, object?>("http.route", RouteDiagnosticsHelpers.ResolveHttpRoute(route)),
             new KeyValuePair<string, object?>("aspnetcore.routing.match_status", "success"),
             new KeyValuePair<string, object?>("aspnetcore.routing.is_fallback", isFallback ? BoxedTrue : BoxedFalse));
     }

src/Http/Routing/test/UnitTests/RoutingMetricsTests.cs

@@ -48,6 +48,34 @@ public async Task Match_Success()
             m => AssertSuccess(m, "/{hi}", fallback: false));
     }
 
+    [Fact]
+    public async Task Match_EmptyRoute_ResolveForwardSlash()
+    {
+        // Arrange
+        var routeEndpointBuilder = new RouteEndpointBuilder(c => Task.CompletedTask, RoutePatternFactory.Parse(string.Empty), order: 0);
+        var meterFactory = new TestMeterFactory();
+        var middleware = CreateMiddleware(
+            matcherFactory: new TestMatcherFactory(true, c =>
+            {
+                c.SetEndpoint(routeEndpointBuilder.Build());
+            }),
+            meterFactory: meterFactory);
+        var httpContext = CreateHttpContext();
+        var meter = meterFactory.Meters.Single();
+
+        using var routingMatchAttemptsCollector = new MetricCollector<long>(meterFactory, RoutingMetrics.MeterName, "aspnetcore.routing.match_attempts");
+
+        // Act
+        await middleware.Invoke(httpContext);
+
+        // Assert
+        Assert.Equal(RoutingMetrics.MeterName, meter.Name);
+        Assert.Null(meter.Version);
+
+        Assert.Collection(routingMatchAttemptsCollector.GetMeasurementSnapshot(),
+            m => AssertSuccess(m, "/", fallback: false));
+    }
+
     [Theory]
     [InlineData(true)]
     [InlineData(false)]

src/Shared/Diagnostics/RouteDiagnosticsHelpers.cs

@@ -0,0 +1,17 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.AspNetCore.Shared;
+
+internal static class RouteDiagnosticsHelpers
+{
+    public static string ResolveHttpRoute(string route)
+    {
+        // A route that matches the root of the website could be an empty string. This is problematic.
+        // 1. It is potentially confusing, "What does empty string mean?"
+        // 2. Some telemetry tools have problems with empty string values, e.g. https://github.com/dotnet/aspnetcore/pull/62432
+        //
+        // The fix is to resolve empty string route to "/" in metrics.
+        return string.IsNullOrEmpty(route) ? "/" : route;
+    }
+}