[Instrumentation.AspNet] Update semantic conventions for metrics

src/OpenTelemetry.Instrumentation.AspNet/CHANGELOG.md

@@ -8,6 +8,32 @@
   * New overload of `AddAspNetInstrumentation` now accepts a configuration delegate.
   * The `Enrich` can be used to add additional metric attributes.
 
+* BREAKING: HTTP server metrics now follow stable
+  [semantic conventions](https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-metrics.md#http-server)
+  ([#1429](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1429)).
+
+  * New metric: `http.server.request.duration`
+    * Unit: `s` (seconds)
+    * Histogram Buckets: `0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5,
+    0.75, 1,  2.5, 5, 7.5, 10`
+  * Old metric: `http.server.duration`
+    * Unit: `ms` (milliseconds)
+    * Histogram Buckets: `0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500,
+    5000, 7500, 10000`
+
+  Note that the bucket changes are part of the 1.7.0-rc.1 release of the
+  `OpenTelemetry` SDK.
+
+  The following metric attributes has been added:
+
+  * `http.request.method` (previously `http.method`)
+  * `http.response.status_code` (previously `http.status_code`)
+  * `url.scheme` (previously `http.scheme`)
+  * `server.address`
+  * `server.port`
+  * `network.protocol.version` (`1.1`, `2`, `3`)
+  * `http.route`
+
 ## 1.6.0-beta.2
 
 Released 2023-Nov-06

src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs

@@ -4,7 +4,6 @@
 using System;
 using System.Diagnostics;
 using System.Web;
-using System.Web.Routing;
 using OpenTelemetry.Context.Propagation;
 using OpenTelemetry.Internal;
 using OpenTelemetry.Trace;
@@ -13,8 +12,7 @@ namespace OpenTelemetry.Instrumentation.AspNet.Implementation;
 
 internal sealed class HttpInListener : IDisposable
 {
-    private readonly PropertyFetcher<object> routeFetcher = new("Route");
-    private readonly PropertyFetcher<string> routeTemplateFetcher = new("RouteTemplate");
+    private readonly HttpRequestRouteHelper routeHelper = new();
     private readonly AspNetInstrumentationOptions options;
 
     public HttpInListener(AspNetInstrumentationOptions options)
@@ -120,26 +118,7 @@ private void OnStopActivity(Activity activity, HttpContext context)
                 activity.SetStatus(SpanHelper.ResolveActivityStatusForHttpStatusCode(activity.Kind, response.StatusCode));
             }
 
-            var routeData = context.Request.RequestContext.RouteData;
-
-            string? template = null;
-            if (routeData.Values.TryGetValue("MS_SubRoutes", out object msSubRoutes))
-            {
-                // WebAPI attribute routing flows here. Use reflection to not take a dependency on microsoft.aspnet.webapi.core\[version]\lib\[framework]\System.Web.Http.
-
-                if (msSubRoutes is Array attributeRouting && attributeRouting.Length == 1)
-                {
-                    var subRouteData = attributeRouting.GetValue(0);
-
-                    _ = this.routeFetcher.TryFetch(subRouteData, out var route);
-                    _ = this.routeTemplateFetcher.TryFetch(route, out template);
-                }
-            }
-            else if (routeData.Route is Route route)
-            {
-                // MVC + WebAPI traditional routing & MVC attribute routing flow here.
-                template = route.Url;
-            }
+            var template = this.routeHelper.GetRouteTemplate(context.Request);
 
             if (!string.IsNullOrEmpty(template))
             {

src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInMetricsListener.cs

@@ -11,12 +11,17 @@ namespace OpenTelemetry.Instrumentation.AspNet.Implementation;
 
 internal sealed class HttpInMetricsListener : IDisposable
 {
+    private readonly HttpRequestRouteHelper routeHelper = new();
+    private readonly RequestMethodHelper requestMethodHelper = new();
     private readonly Histogram<double> httpServerDuration;
     private readonly AspNetMetricsInstrumentationOptions options;
 
     public HttpInMetricsListener(Meter meter, AspNetMetricsInstrumentationOptions options)
     {
-        this.httpServerDuration = meter.CreateHistogram<double>("http.server.duration", "ms", "Measures the duration of inbound HTTP requests.");
+        this.httpServerDuration = meter.CreateHistogram<double>(
+            "http.server.request.duration",
+            unit: "s",
+            description: "Measures the duration of inbound HTTP requests.");
         TelemetryHttpModule.Options.OnRequestStoppedCallback += this.OnStopActivity;
         this.options = options;
     }
@@ -26,17 +31,45 @@ public void Dispose()
         TelemetryHttpModule.Options.OnRequestStoppedCallback -= this.OnStopActivity;
     }
 
+    private static string GetHttpProtocolVersion(HttpRequest request)
+    {
+        var protocol = request.ServerVariables["SERVER_PROTOCOL"];
+        return protocol switch
+        {
+            "HTTP/1.1" => "1.1",
+            "HTTP/2" => "2",
+            "HTTP/3" => "3",
+            _ => protocol,
+        };
+    }
+
     private void OnStopActivity(Activity activity, HttpContext context)
     {
-        // TODO: This is just a minimal set of attributes. See the spec for additional attributes:
-        // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-server
+        var request = context.Request;
+        var url = request.Url;
         var tags = new TagList
         {
-            { SemanticConventions.AttributeHttpMethod, context.Request.HttpMethod },
-            { SemanticConventions.AttributeHttpScheme, context.Request.Url.Scheme },
-            { SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode },
+            { SemanticConventions.AttributeServerAddress, url.Host },
+            { SemanticConventions.AttributeServerPort, url.Port },
+            { SemanticConventions.AttributeUrlScheme, url.Scheme },
+            { SemanticConventions.AttributeHttpResponseStatusCode, context.Response.StatusCode },
         };
 
+        var normalizedMethod = this.requestMethodHelper.GetNormalizedHttpMethod(request.HttpMethod);
+        tags.Add(SemanticConventions.AttributeHttpRequestMethod, normalizedMethod);
+
+        var protocolVersion = GetHttpProtocolVersion(request);
+        if (!string.IsNullOrEmpty(protocolVersion))
+        {
+            tags.Add(SemanticConventions.AttributeNetworkProtocolVersion, protocolVersion);
+        }
+
+        var template = this.routeHelper.GetRouteTemplate(request);
+        if (!string.IsNullOrEmpty(template))
+        {
+            tags.Add(SemanticConventions.AttributeHttpRoute, template);
+        }
+
         if (this.options.Enrich is not null)
         {
             try
@@ -49,6 +82,6 @@ private void OnStopActivity(Activity activity, HttpContext context)
             }
         }
 
-        this.httpServerDuration.Record(activity.Duration.TotalMilliseconds, tags);
+        this.httpServerDuration.Record(activity.Duration.TotalSeconds, tags);
     }
 }

src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpRequestRouteHelper.cs

@@ -0,0 +1,48 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+using System;
+using System.Web;
+using System.Web.Routing;
+
+namespace OpenTelemetry.Instrumentation.AspNet.Implementation;
+
+/// <summary>
+/// Helper class for processing http requests.
+/// </summary>
+internal sealed class HttpRequestRouteHelper
+{
+    private readonly PropertyFetcher<object> routeFetcher = new("Route");
+    private readonly PropertyFetcher<string> routeTemplateFetcher = new("RouteTemplate");
+
+    /// <summary>
+    /// Extracts the route template from the <see cref="HttpRequest"/>.
+    /// </summary>
+    /// <param name="request">The <see cref="HttpRequest"/> being processed.</param>
+    /// <returns>The route template or <see langword="null"/>.</returns>
+    internal string? GetRouteTemplate(HttpRequest request)
+    {
+        var routeData = request.RequestContext.RouteData;
+
+        string? template = null;
+        if (routeData.Values.TryGetValue("MS_SubRoutes", out object msSubRoutes))
+        {
+            // WebAPI attribute routing flows here. Use reflection to not take a dependency on microsoft.aspnet.webapi.core\[version]\lib\[framework]\System.Web.Http.
+
+            if (msSubRoutes is Array attributeRouting && attributeRouting.Length == 1)
+            {
+                var subRouteData = attributeRouting.GetValue(0);
+
+                _ = this.routeFetcher.TryFetch(subRouteData, out var route);
+                _ = this.routeTemplateFetcher.TryFetch(route, out template);
+            }
+        }
+        else if (routeData.Route is Route route)
+        {
+            // MVC + WebAPI traditional routing & MVC attribute routing flow here.
+            template = route.Url;
+        }
+
+        return template;
+    }
+}

src/OpenTelemetry.Instrumentation.AspNet/Implementation/RequestMethodHelper.cs

@@ -0,0 +1,49 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+
+namespace OpenTelemetry.Instrumentation.AspNet.Implementation;
+
+internal sealed class RequestMethodHelper
+{
+    private const string KnownHttpMethodsEnvironmentVariable = "OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS";
+
+    // The value "_OTHER" is used for non-standard HTTP methods.
+    // https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md#common-attributes
+    private const string OtherHttpMethod = "_OTHER";
+
+    private static readonly char[] SplitChars = new[] { ',' };
+
+    // List of known HTTP methods as per spec.
+    private readonly Dictionary<string, string> knownHttpMethods;
+
+    public RequestMethodHelper()
+    {
+        var suppliedKnownMethods = Environment.GetEnvironmentVariable(KnownHttpMethodsEnvironmentVariable)
+            ?.Split(SplitChars, StringSplitOptions.RemoveEmptyEntries);
+        this.knownHttpMethods = suppliedKnownMethods?.Length > 0
+            ? suppliedKnownMethods.ToDictionary(x => x, x => x, StringComparer.OrdinalIgnoreCase)
+            : new(StringComparer.OrdinalIgnoreCase)
+            {
+                ["GET"] = "GET",
+                ["POST"] = "POST",
+                ["PUT"] = "PUT",
+                ["DELETE"] = "DELETE",
+                ["HEAD"] = "HEAD",
+                ["OPTIONS"] = "OPTIONS",
+                ["TRACE"] = "TRACE",
+                ["PATCH"] = "PATCH",
+                ["CONNECT"] = "CONNECT",
+            };
+    }
+
+    public string GetNormalizedHttpMethod(string method)
+    {
+        return this.knownHttpMethods.TryGetValue(method, out var normalizedMethod)
+            ? normalizedMethod
+            : OtherHttpMethod;
+    }
+}

src/Shared/SemanticConventions.cs

@@ -101,5 +101,11 @@ internal static class SemanticConventions
     public const string AttributeHttpRequestMethod = "http.request.method"; // replaces: "http.method" (AttributeHttpMethod)
     public const string AttributeHttpResponseStatusCode = "http.response.status_code"; // replaces: "http.status_code" (AttributeHttpStatusCode)
     public const string AttributeUrlScheme = "url.scheme"; // replaces: "http.scheme" (AttributeHttpScheme)
+
+    // v1.23.0
+    // https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-metrics.md#http-server
+    public const string AttributeNetworkProtocolVersion = "network.protocol.version"; // replaces: "http.flavor" (AttributeHttpFlavor)
+    public const string AttributeServerAddress = "server.address"; // replaces: "net.host.name" (AttributeNetHostName)
+    public const string AttributeServerPort = "server.port"; // replaces: "net.host.port" (AttributeNetHostPort)
 #pragma warning restore CS1591 // Missing XML comment for publicly visible type or member
 }

test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInMetricsListenerTests.cs

@@ -16,27 +16,33 @@ namespace OpenTelemetry.Instrumentation.AspNet.Tests;
 public class HttpInMetricsListenerTests
 {
     [Theory]
-    [InlineData("http://localhost/", 0, null, null, "http")]
-    [InlineData("https://localhost/", 0, null, null, "https")]
-    [InlineData("http://localhost/api/value", 0, null, null, "http")]
-    [InlineData("http://localhost/api/value", 1, "{controller}/{action}", null, "http")]
-    [InlineData("http://localhost/api/value", 2, "{controller}/{action}", null, "http")]
-    [InlineData("http://localhost/api/value", 3, "{controller}/{action}", null, "http")]
-    [InlineData("http://localhost/api/value", 4, "{controller}/{action}", null, "http")]
-    [InlineData("http://localhost:8080/api/value", 0, null, null, "http")]
-    [InlineData("http://localhost:8080/api/value", 1, "{controller}/{action}", null, "http")]
-    [InlineData("http://localhost:8080/api/value", 3, "{controller}/{action}", "enrich", "http")]
-    [InlineData("http://localhost:8080/api/value", 3, "{controller}/{action}", "throw", "http")]
-    [InlineData("http://localhost:8080/api/value", 3, "{controller}/{action}", null, "http")]
+    [InlineData("http://localhost/", 0, null, null, "http", "localhost", null, 80, 200)]
+    [InlineData("https://localhost/", 0, null, null, "https", "localhost", null, 443, 200)]
+    [InlineData("http://localhost/api/value", 0, null, null, "http", "localhost", null, 80, 200)]
+    [InlineData("http://localhost/api/value", 1, "{controller}/{action}", null, "http", "localhost", "{controller}/{action}", 80, 200)]
+    [InlineData("http://localhost/api/value", 2, "{controller}/{action}", null, "http", "localhost", "{controller}/{action}", 80, 201)]
+    [InlineData("http://localhost/api/value", 3, "{controller}/{action}", null, "http", "localhost", "{controller}/{action}", 80, 200)]
+    [InlineData("http://localhost/api/value", 4, "{controller}/{action}", null, "http", "localhost", "{controller}/{action}", 80, 200)]
+    [InlineData("http://localhost/api/value", 1, "{controller}/{action}", null, "http", "localhost", "{controller}/{action}", 80, 500)]
+    [InlineData("http://localhost:8080/api/value", 0, null, null, "http", "localhost", null, 8080, 200)]
+    [InlineData("http://localhost:8080/api/value", 1, "{controller}/{action}", null, "http", "localhost", "{controller}/{action}", 8080, 200)]
+    [InlineData("http://localhost:8080/api/value", 3, "{controller}/{action}", "enrich", "http", "localhost", "{controller}/{action}", 8080, 200)]
+    [InlineData("http://localhost:8080/api/value", 3, "{controller}/{action}", "throw", "http", "localhost", "{controller}/{action}", 8080, 200)]
+    [InlineData("http://localhost:8080/api/value", 3, "{controller}/{action}", null, "http", "localhost", "{controller}/{action}", 8080, 200)]
     public void AspNetMetricTagsAreCollectedSuccessfully(
         string url,
         int routeType,
         string routeTemplate,
         string enrichMode,
-        string expectedScheme)
+        string expectedScheme,
+        string expectedHost,
+        string expectedRoute,
+        int? expectedPort,
+        int expectedStatus)
     {
         double duration = 0;
         HttpContext.Current = RouteTestHelper.BuildHttpContext(url, routeType, routeTemplate);
+        HttpContext.Current.Response.StatusCode = expectedStatus;
 
         // This is to enable activity creation
         // as it is created using ActivitySource inside TelemetryHttpModule
@@ -47,7 +53,7 @@ public void AspNetMetricTagsAreCollectedSuccessfully(
                 {
                     if (eventName.Equals("OnStopActivity"))
                     {
-                        duration = activity.Duration.TotalMilliseconds;
+                        duration = activity.Duration.TotalSeconds;
                     }
                 })
             .Build();
@@ -94,12 +100,19 @@ public void AspNetMetricTagsAreCollectedSuccessfully(
         var sum = metricPoint.GetHistogramSum();
 
         Assert.Equal(MetricType.Histogram, exportedItems[0].MetricType);
-        Assert.Equal("http.server.duration", exportedItems[0].Name);
+        Assert.Equal("http.server.request.duration", exportedItems[0].Name);
+        Assert.Equal("s", exportedItems[0].Unit);
         Assert.Equal(1L, count);
         Assert.Equal(duration, sum);
         Assert.True(duration > 0, "Metric duration should be set.");
 
-        var expectedTagCount = 3;
+        var expectedTagCount = 5;
+
+        if (!string.IsNullOrEmpty(expectedRoute))
+        {
+            expectedTagCount++;
+        }
+
         if (enrichMode == "enrich")
         {
             expectedTagCount++;
@@ -117,9 +130,28 @@ public void AspNetMetricTagsAreCollectedSuccessfully(
             ExpectTag("true", "enriched");
         }
 
-        ExpectTag("GET", SemanticConventions.AttributeHttpMethod);
-        ExpectTag(200, SemanticConventions.AttributeHttpStatusCode);
-        ExpectTag(expectedScheme, SemanticConventions.AttributeHttpScheme);
+        // Do not use constants from SemanticConventions here in order to detect mistakes.
+        // https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-metrics.md#http-server
+        // Unable to check for "network.protocol.version" because we can't set server variables due to the accessibility
+        // of the ServerVariables property.
+        ExpectTag("GET", "http.request.method");
+        ExpectTag(expectedStatus, "http.response.status_code");
+        ExpectTag(expectedRoute, "http.route");
+        ExpectTag(expectedHost, "server.address");
+        ExpectTag(expectedPort, "server.port");
+        ExpectTag(expectedScheme, "url.scheme");
+
+        // Inspect histogram bucket boundaries.
+        var histogramBuckets = metricPoint.GetHistogramBuckets();
+        var histogramBounds = new List<double>();
+        foreach (var t in histogramBuckets)
+        {
+            histogramBounds.Add(t.ExplicitBound);
+        }
+
+        Assert.Equal(
+            expected: new List<double> { 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10, double.PositiveInfinity },
+            actual: histogramBounds);
 
         void ExpectTag<T>(T? expected, string tagName)
         {