HttpSemanticConventions - new metric in AspNetCore Instrumentation.

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+* TODO: UPDATE THIS
+
 ## 1.5.1-beta.1
 
 Released 2023-Jul-20

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

@@ -27,13 +27,16 @@ namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation;
 
 internal sealed class HttpInMetricsListener : ListenerHandler
 {
-    private const string HttpServerDurationMetricName = "http.server.duration";
+    internal const string HttpServerDurationMetricName = "http.server.duration";
+    internal const string HttpServerRequestDurationMetricName = "http.server.request.duration";
+
     private const string OnStopEvent = "Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop";
     private const string EventName = "OnStopActivity";
 
     private readonly Meter meter;
     private readonly AspNetCoreMetricsInstrumentationOptions options;
     private readonly Histogram<double> httpServerDuration;
+    private readonly Histogram<double> httpServerRequestDuration;
     private readonly bool emitOldAttributes;
     private readonly bool emitNewAttributes;
 
@@ -44,6 +47,10 @@ internal HttpInMetricsListener(string name, Meter meter, AspNetCoreMetricsInstru
         this.options = options;
         this.httpServerDuration = meter.CreateHistogram<double>(HttpServerDurationMetricName, "ms", "Measures the duration of inbound HTTP requests.");
 
+        // TODO: This needs to be changed to  "s" (seconds). This is blocked until we can change the default histogram.
+        // See: https://github.com/open-telemetry/opentelemetry-dotnet/issues/4797
+        this.httpServerRequestDuration = meter.CreateHistogram<double>(HttpServerRequestDurationMetricName, "ms", "Measures the duration of inbound HTTP requests.");
+
         this.emitOldAttributes = this.options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old);
 
         this.emitNewAttributes = this.options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.New);
@@ -110,16 +117,6 @@ public override void OnEventWritten(string name, object payload)
                 tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeUrlScheme, context.Request.Scheme));
                 tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRequestMethod, context.Request.Method));
                 tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode)));
-
-                if (context.Request.Host.HasValue)
-                {
-                    tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeServerAddress, context.Request.Host.Host));
-
-                    if (context.Request.Host.Port is not null && context.Request.Host.Port != 80 && context.Request.Host.Port != 443)
-                    {
-                        tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeServerPort, context.Request.Host.Port));
-                    }
-                }
             }
 
 #if NET6_0_OR_GREATER
@@ -144,7 +141,17 @@ public override void OnEventWritten(string name, object payload)
             // We are relying here on ASP.NET Core to set duration before writing the stop event.
             // https://github.com/dotnet/aspnetcore/blob/d6fa351048617ae1c8b47493ba1abbe94c3a24cf/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L449
             // TODO: Follow up with .NET team if we can continue to rely on this behavior.
-            this.httpServerDuration.Record(Activity.Current.Duration.TotalMilliseconds, tags);
+            if (this.emitNewAttributes)
+            {
+                // TODO: This needs to be changed to TotalSeconds. This is blocked until we can change the default histogram.
+                // See: https://github.com/open-telemetry/opentelemetry-dotnet/issues/4797
+                this.httpServerRequestDuration.Record(Activity.Current.Duration.TotalMilliseconds, tags);
+            }
+
+            if (this.emitOldAttributes)
+            {
+                this.httpServerDuration.Record(Activity.Current.Duration.TotalMilliseconds, tags);
+            }
         }
     }
 }

src/OpenTelemetry.Instrumentation.AspNetCore/README.md

@@ -90,13 +90,22 @@ public void ConfigureServices(IServiceCollection services)
 
 #### List of metrics produced
 
-The instrumentation is implemented based on [metrics semantic
-conventions](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#metric-httpserverduration).
-Currently, the instrumentation supports the following metric.
+The instrumentation is implemented based on
+[metrics semantic conventions](https://github.com/open-telemetry/semantic-conventions/blob/4bbb8c907402caa90bc077214e8a2c78807c1ab9/docs/http/http-metrics.md).
 
-| Name  | Instrument Type | Unit | Description |
-|-------|-----------------|------|-------------|
-| `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. |
+Have you opt-ed into the new Http Semantic Conventions?
+
+* If yes, the instrumentation supports the following metric.
+
+    | Name  | Instrument Type | Unit | Description |
+    |-------|-----------------|------|-------------|
+    | `http.server.request.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. |
+
+* If no, the instrumentation supports the following metric.
+
+    | Name  | Instrument Type | Unit | Description |
+    |-------|-----------------|------|-------------|
+    | `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. |
 
 ## Advanced configuration
 

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs

@@ -30,6 +30,8 @@ namespace OpenTelemetry.Instrumentation.AspNetCore.Tests;
 public class MetricTests
     : IClassFixture<WebApplicationFactory<Program>>, IDisposable
 {
+    public const string SemanticConventionOptInKeyName = "OTEL_SEMCONV_STABILITY_OPT_IN";
+
     private const int StandardTagsCount = 6;
 
     private readonly WebApplicationFactory<Program> factory;
@@ -47,47 +49,81 @@ public void AddAspNetCoreInstrumentation_BadArgs()
         Assert.Throws<ArgumentNullException>(() => builder.AddAspNetCoreInstrumentation());
     }
 
-    [Fact]
-    public async Task RequestMetricIsCaptured()
+    [Theory]
+    [InlineData(null, true, false, 6)] // emits old metric & attributes
+    [InlineData("http", false, true, 5)] // emits new metric & attributes
+    [InlineData("http/dup", true, true, 10)] // emits both old & new
+    public async Task RequestMetricIsCaptured(string environmentVarValue, bool validateOldSemConv, bool validateNewSemConv, int expectedTagsCount)
     {
-        var metricItems = new List<Metric>();
+        try
+        {
+            Environment.SetEnvironmentVariable(SemanticConventionOptInKeyName, environmentVarValue);
 
-        this.meterProvider = Sdk.CreateMeterProviderBuilder()
-            .AddAspNetCoreInstrumentation()
-            .AddInMemoryExporter(metricItems)
-            .Build();
+            var metricItems = new List<Metric>();
 
-        using (var client = this.factory
-            .WithWebHostBuilder(builder =>
+            this.meterProvider = Sdk.CreateMeterProviderBuilder()
+                .AddAspNetCoreInstrumentation()
+                .AddInMemoryExporter(metricItems)
+                .Build();
+
+            using (var client = this.factory
+                .WithWebHostBuilder(builder =>
+                {
+                    builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders());
+                })
+                .CreateClient())
             {
-                builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders());
-            })
-            .CreateClient())
-        {
-            using var response1 = await client.GetAsync("/api/values").ConfigureAwait(false);
-            using var response2 = await client.GetAsync("/api/values/2").ConfigureAwait(false);
+                using var response1 = await client.GetAsync("/api/values").ConfigureAwait(false);
+                using var response2 = await client.GetAsync("/api/values/2").ConfigureAwait(false);
 
-            response1.EnsureSuccessStatusCode();
-            response2.EnsureSuccessStatusCode();
-        }
+                response1.EnsureSuccessStatusCode();
+                response2.EnsureSuccessStatusCode();
+            }
 
-        // We need to let End callback execute as it is executed AFTER response was returned.
-        // In unit tests environment there may be a lot of parallel unit tests executed, so
-        // giving some breezing room for the End callback to complete
-        await Task.Delay(TimeSpan.FromSeconds(1)).ConfigureAwait(false);
+            // We need to let End callback execute as it is executed AFTER response was returned.
+            // In unit tests environment there may be a lot of parallel unit tests executed, so
+            // giving some breezing room for the End callback to complete
+            await Task.Delay(TimeSpan.FromSeconds(1)).ConfigureAwait(false);
 
-        this.meterProvider.Dispose();
+            this.meterProvider.Dispose();
 
-        var requestMetrics = metricItems
-            .Where(item => item.Name == "http.server.duration")
-            .ToArray();
+            if (validateOldSemConv)
+            {
+                var requestMetrics = metricItems
+                    .Where(item => item.Name == "http.server.duration")
+                    .ToArray();
 
-        var metric = Assert.Single(requestMetrics);
-        var metricPoints = GetMetricPoints(metric);
-        Assert.Equal(2, metricPoints.Count);
+                var metric = Assert.Single(requestMetrics);
+                Assert.Equal("ms", metric.Unit);
+                var metricPoints = GetMetricPoints(metric);
+                Assert.Equal(2, metricPoints.Count);
+
+                AssertMetricPoint(metricPoints[0], expectedRoute: "api/Values", expectedTagsCount: expectedTagsCount, validateOldSemConv: true);
+                AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}", expectedTagsCount: expectedTagsCount, validateOldSemConv: true);
+            }
 
-        AssertMetricPoint(metricPoints[0], expectedRoute: "api/Values");
-        AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}");
+            if (validateNewSemConv)
+            {
+                var requestMetrics = metricItems
+                    .Where(item => item.Name == "http.server.request.duration")
+                    .ToArray();
+
+                var metric = Assert.Single(requestMetrics);
+
+                // TODO: This needs to be changed to "s" (seconds). This is blocked until we can change the default histogram.
+                // See: https://github.com/open-telemetry/opentelemetry-dotnet/issues/4797
+                Assert.Equal("ms", metric.Unit);
+                var metricPoints = GetMetricPoints(metric);
+                Assert.Equal(2, metricPoints.Count);
+
+                AssertMetricPoint(metricPoints[0], expectedRoute: "api/Values", expectedTagsCount: expectedTagsCount, validateNewSemConv: true);
+                AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}", expectedTagsCount: expectedTagsCount, validateNewSemConv: true);
+            }
+        }
+        finally
+        {
+            Environment.SetEnvironmentVariable(SemanticConventionOptInKeyName, null);
+        }
     }
 
     [Fact]
@@ -133,7 +169,7 @@ void ConfigureTestServices(IServiceCollection services)
 
         // Assert single because we filtered out one route
         var metricPoint = Assert.Single(GetMetricPoints(metric));
-        AssertMetricPoint(metricPoint);
+        AssertMetricPoint(metricPoint, validateOldSemConv: true);
     }
 
     [Fact]
@@ -187,7 +223,7 @@ void ConfigureTestServices(IServiceCollection services)
         var metric = Assert.Single(requestMetrics);
         var metricPoint = Assert.Single(GetMetricPoints(metric));
 
-        var tags = AssertMetricPoint(metricPoint, expectedTagsCount: StandardTagsCount + 2);
+        var tags = AssertMetricPoint(metricPoint, expectedTagsCount: StandardTagsCount + 2, validateOldSemConv: true);
 
         Assert.Contains(tagsToAdd[0], tags);
         Assert.Contains(tagsToAdd[1], tags);
@@ -215,7 +251,9 @@ private static List<MetricPoint> GetMetricPoints(Metric metric)
     private static KeyValuePair<string, object>[] AssertMetricPoint(
         MetricPoint metricPoint,
         string expectedRoute = "api/Values",
-        int expectedTagsCount = StandardTagsCount)
+        int expectedTagsCount = StandardTagsCount,
+        bool validateNewSemConv = false,
+        bool validateOldSemConv = false)
     {
         var count = metricPoint.GetHistogramCount();
         var sum = metricPoint.GetHistogramSum();
@@ -230,20 +268,53 @@ private static KeyValuePair<string, object>[] AssertMetricPoint(
             attributes[i++] = tag;
         }
 
-        var method = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpMethod, "GET");
-        var scheme = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpScheme, "http");
-        var statusCode = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, 200);
-        var flavor = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpFlavor, "1.1");
-        var host = new KeyValuePair<string, object>(SemanticConventions.AttributeNetHostName, "localhost");
-        var route = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRoute, expectedRoute);
-        Assert.Contains(method, attributes);
-        Assert.Contains(scheme, attributes);
-        Assert.Contains(statusCode, attributes);
-        Assert.Contains(flavor, attributes);
-        Assert.Contains(host, attributes);
-        Assert.Contains(route, attributes);
+        // Inspect Attributes
         Assert.Equal(expectedTagsCount, attributes.Length);
 
+        if (validateNewSemConv)
+        {
+            var method = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRequestMethod, "GET");
+            var scheme = new KeyValuePair<string, object>(SemanticConventions.AttributeUrlScheme, "http");
+            var statusCode = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpResponseStatusCode, 200);
+            var flavor = new KeyValuePair<string, object>(SemanticConventions.AttributeNetworkProtocolVersion, "1.1");
+            var route = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRoute, expectedRoute);
+            Assert.Contains(method, attributes);
+            Assert.Contains(scheme, attributes);
+            Assert.Contains(statusCode, attributes);
+            Assert.Contains(flavor, attributes);
+            Assert.Contains(route, attributes);
+        }
+
+        if (validateOldSemConv)
+        {
+            var method = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpMethod, "GET");
+            var scheme = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpScheme, "http");
+            var statusCode = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, 200);
+            var flavor = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpFlavor, "1.1");
+            var host = new KeyValuePair<string, object>(SemanticConventions.AttributeNetHostName, "localhost");
+            var route = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRoute, expectedRoute);
+            Assert.Contains(method, attributes);
+            Assert.Contains(scheme, attributes);
+            Assert.Contains(statusCode, attributes);
+            Assert.Contains(flavor, attributes);
+            Assert.Contains(host, attributes);
+            Assert.Contains(route, attributes);
+        }
+
+        // Inspect Histogram Bounds
+        var histogramBuckets = metricPoint.GetHistogramBuckets();
+        var histogramBounds = new List<double>();
+        foreach (var t in histogramBuckets)
+        {
+            histogramBounds.Add(t.ExplicitBound);
+        }
+
+        // TODO: This will need to test for the new histograms. This is blocked until we can change the default histogram.
+        // See: https://github.com/open-telemetry/opentelemetry-dotnet/issues/4797
+        Assert.Equal(
+            expected: new List<double> { 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000, double.PositiveInfinity },
+            actual: histogramBounds);
+
         return attributes;
     }
 }