From 91f08c856f0ea843dfb00b650a3d3328aeaa03ec Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 24 Aug 2023 10:19:53 -0700 Subject: [PATCH 01/40] update AspNetCore for new metric 'http.server.request.duration' --- .../CHANGELOG.md | 2 + .../Implementation/HttpInMetricsListener.cs | 19 ++- .../MeterProviderBuilderExtensions.cs | 2 + .../README.md | 6 +- .../MetricTests.cs | 161 +++++++++++++----- 5 files changed, 140 insertions(+), 50 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index b44b5fb8b1e..6316d096e0e 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +* TODO: UPDATE THIS + ## 1.5.1-beta.1 Released 2023-Jul-20 diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index ce06402bc34..7b15b0f43f9 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/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 httpServerDuration; + private readonly Histogram httpServerRequestDuration; private readonly bool emitOldAttributes; private readonly bool emitNewAttributes; @@ -44,6 +47,9 @@ internal HttpInMetricsListener(string name, Meter meter, AspNetCoreMetricsInstru this.options = options; this.httpServerDuration = meter.CreateHistogram(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. + this.httpServerRequestDuration = meter.CreateHistogram(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); @@ -144,7 +150,16 @@ 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. + this.httpServerRequestDuration.Record(Activity.Current.Duration.TotalMilliseconds, tags); + } + + if (this.emitOldAttributes) + { + this.httpServerDuration.Record(Activity.Current.Duration.TotalMilliseconds, tags); + } } } } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs index 99330ac220a..3a8e0dacf1a 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs @@ -14,11 +14,13 @@ // limitations under the License. // +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using OpenTelemetry.Instrumentation.AspNetCore; using OpenTelemetry.Instrumentation.AspNetCore.Implementation; using OpenTelemetry.Internal; +using static OpenTelemetry.Internal.HttpSemanticConventionHelper; namespace OpenTelemetry.Metrics; diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index 131f98a4b37..4df2d73964b 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -90,13 +90,15 @@ public void ConfigureServices(IServiceCollection services) #### List of metrics produced +TODO: UPDATE THIS + 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). +conventions](https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-metrics.md#metric-httpserverduration). Currently, the instrumentation supports the following metric. | Name | Instrument Type | Unit | Description | |-------|-----------------|------|-------------| -| `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. | +| `http.server.duration` | Histogram | `s` | Measures the duration of inbound HTTP requests. | ## Advanced configuration diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index 607472c83a4..f430e1188b5 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -30,6 +30,8 @@ namespace OpenTelemetry.Instrumentation.AspNetCore.Tests; public class MetricTests : IClassFixture>, IDisposable { + public const string SemanticConventionOptInKeyName = "OTEL_SEMCONV_STABILITY_OPT_IN"; + private const int StandardTagsCount = 6; private readonly WebApplicationFactory factory; @@ -47,47 +49,78 @@ public void AddAspNetCoreInstrumentation_BadArgs() Assert.Throws(() => builder.AddAspNetCoreInstrumentation()); } - [Fact] - public async Task RequestMetricIsCaptured() + [Theory] + [InlineData(null, true, false, 6)] // emits old metric & attributes + [InlineData("http", false, true, 6)] // emits new metric & attributes + [InlineData("http/dup", true, true, 11)] // emits both old & new + public async Task RequestMetricIsCaptured(string environmentVarValue, bool validateOldSemConv, bool validateNewSemConv, int expectedTagsCount) { - var metricItems = new List(); + try + { + Environment.SetEnvironmentVariable(SemanticConventionOptInKeyName, environmentVarValue); - this.meterProvider = Sdk.CreateMeterProviderBuilder() - .AddAspNetCoreInstrumentation() - .AddInMemoryExporter(metricItems) - .Build(); + var metricItems = new List(); - 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"); - AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}"); + AssertMetricPoint(metricPoints[0], expectedRoute: "api/Values", expectedTagsCount: expectedTagsCount, validateOldSemConv: true); + AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}", expectedTagsCount: expectedTagsCount, validateOldSemConv: true); + } + + if (validateNewSemConv) + { + var requestMetrics = metricItems + .Where(item => item.Name == "http.server.request.duration") + .ToArray(); + + var metric = Assert.Single(requestMetrics); + Assert.Equal("ms", metric.Unit); // TODO: This needs to be changed to "s" (seconds). This is blocked until we can change the default histogram. + 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 +166,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 +220,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 +248,9 @@ private static List GetMetricPoints(Metric metric) private static KeyValuePair[] 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 +265,54 @@ private static KeyValuePair[] AssertMetricPoint( attributes[i++] = tag; } - var method = new KeyValuePair(SemanticConventions.AttributeHttpMethod, "GET"); - var scheme = new KeyValuePair(SemanticConventions.AttributeHttpScheme, "http"); - var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, 200); - var flavor = new KeyValuePair(SemanticConventions.AttributeHttpFlavor, "1.1"); - var host = new KeyValuePair(SemanticConventions.AttributeNetHostName, "localhost"); - var route = new KeyValuePair(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(SemanticConventions.AttributeHttpRequestMethod, "GET"); + var scheme = new KeyValuePair(SemanticConventions.AttributeUrlScheme, "http"); + var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, 200); + var flavor = new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, "1.1"); + var host = new KeyValuePair(SemanticConventions.AttributeServerAddress, "localhost"); + var route = new KeyValuePair(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); + } + + if (validateOldSemConv) + { + var method = new KeyValuePair(SemanticConventions.AttributeHttpMethod, "GET"); + var scheme = new KeyValuePair(SemanticConventions.AttributeHttpScheme, "http"); + var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, 200); + var flavor = new KeyValuePair(SemanticConventions.AttributeHttpFlavor, "1.1"); + var host = new KeyValuePair(SemanticConventions.AttributeNetHostName, "localhost"); + var route = new KeyValuePair(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(); + 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. + Assert.Equal( + expected: new List { 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000, double.PositiveInfinity }, + actual: histogramBounds); + return attributes; } } From c111eead290c7dc3f6c9e798d0e73723dedac287 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 24 Aug 2023 14:49:26 -0700 Subject: [PATCH 02/40] cleanup --- .../MeterProviderBuilderExtensions.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs index 3a8e0dacf1a..99330ac220a 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs @@ -14,13 +14,11 @@ // limitations under the License. // -using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using OpenTelemetry.Instrumentation.AspNetCore; using OpenTelemetry.Instrumentation.AspNetCore.Implementation; using OpenTelemetry.Internal; -using static OpenTelemetry.Internal.HttpSemanticConventionHelper; namespace OpenTelemetry.Metrics; From 32d894674a1f2fd598ad0379d90210f7d74213b4 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 24 Aug 2023 14:56:03 -0700 Subject: [PATCH 03/40] remove attributes --- .../Implementation/HttpInMetricsListener.cs | 10 ---------- .../MetricTests.cs | 6 ++---- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 7b15b0f43f9..9400c9781c4 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -116,16 +116,6 @@ public override void OnEventWritten(string name, object payload) tags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, context.Request.Scheme)); tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, context.Request.Method)); tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); - - if (context.Request.Host.HasValue) - { - tags.Add(new KeyValuePair(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(SemanticConventions.AttributeServerPort, context.Request.Host.Port)); - } - } } #if NET6_0_OR_GREATER diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index f430e1188b5..d52174886a6 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -51,8 +51,8 @@ public void AddAspNetCoreInstrumentation_BadArgs() [Theory] [InlineData(null, true, false, 6)] // emits old metric & attributes - [InlineData("http", false, true, 6)] // emits new metric & attributes - [InlineData("http/dup", true, true, 11)] // emits both old & new + [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) { try @@ -274,13 +274,11 @@ private static KeyValuePair[] AssertMetricPoint( var scheme = new KeyValuePair(SemanticConventions.AttributeUrlScheme, "http"); var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, 200); var flavor = new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, "1.1"); - var host = new KeyValuePair(SemanticConventions.AttributeServerAddress, "localhost"); var route = new KeyValuePair(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); } From 32084319c3883f29b846b46f62c4b2d99cdd5e22 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 24 Aug 2023 15:01:24 -0700 Subject: [PATCH 04/40] comment --- .../Implementation/HttpInMetricsListener.cs | 2 ++ .../MetricTests.cs | 1 + 2 files changed, 3 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 9400c9781c4..582e0c123e2 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -48,6 +48,7 @@ internal HttpInMetricsListener(string name, Meter meter, AspNetCoreMetricsInstru this.httpServerDuration = meter.CreateHistogram(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(HttpServerRequestDurationMetricName, "ms", "Measures the duration of inbound HTTP requests."); this.emitOldAttributes = this.options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old); @@ -143,6 +144,7 @@ public override void OnEventWritten(string name, object payload) 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); } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index d52174886a6..3a2c8e138fd 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -307,6 +307,7 @@ private static KeyValuePair[] AssertMetricPoint( } // 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 { 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000, double.PositiveInfinity }, actual: histogramBounds); From d2f7ef41dbfa4dc2659305e4db1d809b28777f08 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 24 Aug 2023 15:07:17 -0700 Subject: [PATCH 05/40] readme --- .../README.md | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index 4df2d73964b..735a19ecab5 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -90,15 +90,21 @@ public void ConfigureServices(IServiceCollection services) #### List of metrics produced -TODO: UPDATE THIS - The instrumentation is implemented based on [metrics semantic -conventions](https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-metrics.md#metric-httpserverduration). +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. | Name | Instrument Type | Unit | Description | |-------|-----------------|------|-------------| -| `http.server.duration` | Histogram | `s` | Measures the duration of inbound HTTP requests. | +| `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. | + + +If you've opt-ed into the [newer Semantic Conventions](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverduration), +the instrumentation supports the following metric. + +| Name | Instrument Type | Unit | Description | +|-------|-----------------|------|-------------| +| `http.server.request.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. | ## Advanced configuration From 1a49dabe806d891c2b7b8d4a63997e7556297642 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 24 Aug 2023 15:34:32 -0700 Subject: [PATCH 06/40] comment --- .../MetricTests.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index 3a2c8e138fd..5455e070547 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -109,7 +109,10 @@ public async Task RequestMetricIsCaptured(string environmentVarValue, bool valid .ToArray(); var metric = Assert.Single(requestMetrics); - Assert.Equal("ms", metric.Unit); // TODO: This needs to be changed to "s" (seconds). This is blocked until we can change the default histogram. + + // 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); From 7c11551aa460f723532220105682b38be763abbe Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 24 Aug 2023 15:48:31 -0700 Subject: [PATCH 07/40] update readme --- .../README.md | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index 735a19ecab5..9130c726b9a 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -90,21 +90,24 @@ 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. -If you've opt-ed into the [newer Semantic Conventions](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverduration), -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, instrumentation supports the following metric. + + | Name | Instrument Type | Unit | Description | + |-------|-----------------|------|-------------| + | `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. | -| Name | Instrument Type | Unit | Description | -|-------|-----------------|------|-------------| -| `http.server.request.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. | ## Advanced configuration From 93fe6ad478c690af0b7436e0972781688199d81e Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 24 Aug 2023 15:51:35 -0700 Subject: [PATCH 08/40] cleanup --- src/OpenTelemetry.Instrumentation.AspNetCore/README.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index 9130c726b9a..6f9f3324834 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -95,20 +95,18 @@ The instrumentation is implemented based on Have you opt-ed into the new Http Semantic Conventions? -- If yes, the instrumentation supports the following metric. +* 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, instrumentation supports the following metric. +* 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 This instrumentation can be configured to change the default behavior by using From 3842aa8e05478ede0be3007fc1bed8fc2f4e217b Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Fri, 25 Aug 2023 09:40:03 -0700 Subject: [PATCH 09/40] check flag in ctor --- .../Implementation/HttpInMetricsListener.cs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 582e0c123e2..a1394fe8a8e 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -45,15 +45,22 @@ internal HttpInMetricsListener(string name, Meter meter, AspNetCoreMetricsInstru { this.meter = meter; this.options = options; - this.httpServerDuration = meter.CreateHistogram(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(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); + + if (this.emitOldAttributes) + { + this.httpServerDuration = meter.CreateHistogram(HttpServerDurationMetricName, "ms", "Measures the duration of inbound HTTP requests."); + } + + if (this.emitNewAttributes) + { + // 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(HttpServerRequestDurationMetricName, "ms", "Measures the duration of inbound HTTP requests."); + } } public override void OnEventWritten(string name, object payload) From 5f74488cc00ba88a02314810ec2d0dd3b82af58d Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 29 Aug 2023 14:23:28 -0700 Subject: [PATCH 10/40] update tags on metrics --- .../Implementation/HttpInMetricsListener.cs | 46 +++++++++++++------ .../MetricTests.cs | 16 +++---- 2 files changed, 39 insertions(+), 23 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index a1394fe8a8e..f64b21ac7e0 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -96,23 +96,23 @@ public override void OnEventWritten(string name, object payload) return; } - TagList tags = default; + TagList oldTags = default, newTags = default; // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md if (this.emitOldAttributes) { - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, context.Request.Scheme)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, context.Request.Method)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); + oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); + oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, context.Request.Scheme)); + oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, context.Request.Method)); + oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); if (context.Request.Host.HasValue) { - tags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostName, context.Request.Host.Host)); + oldTags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostName, 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(SemanticConventions.AttributeNetHostPort, context.Request.Host.Port)); + oldTags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostPort, context.Request.Host.Port)); } } } @@ -120,24 +120,40 @@ public override void OnEventWritten(string name, object payload) // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md if (this.emitNewAttributes) { - tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); - tags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, context.Request.Scheme)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, context.Request.Method)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); + newTags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); + newTags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, context.Request.Scheme)); + newTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, context.Request.Method)); + newTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); } #if NET6_0_OR_GREATER var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; if (!string.IsNullOrEmpty(route)) { - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); + if (this.emitOldAttributes) + { + oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); + } + + if (this.emitNewAttributes) + { + newTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); + } } #endif if (this.options.Enrich != null) { try { - this.options.Enrich(HttpServerDurationMetricName, context, ref tags); + if (this.emitOldAttributes) + { + this.options.Enrich(HttpServerDurationMetricName, context, ref oldTags); + } + + if (this.emitNewAttributes) + { + this.options.Enrich(HttpServerDurationMetricName, context, ref newTags); + } } catch (Exception ex) { @@ -152,12 +168,12 @@ public override void OnEventWritten(string name, object payload) { // 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); + this.httpServerRequestDuration.Record(Activity.Current.Duration.TotalMilliseconds, newTags); } if (this.emitOldAttributes) { - this.httpServerDuration.Record(Activity.Current.Duration.TotalMilliseconds, tags); + this.httpServerDuration.Record(Activity.Current.Duration.TotalMilliseconds, oldTags); } } } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index 5455e070547..57576c23ccc 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -50,10 +50,10 @@ public void AddAspNetCoreInstrumentation_BadArgs() } [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) + [InlineData(null, true, false, 6, 0)] // emits old metric & attributes + [InlineData("http", false, true, 0, 5)] // emits new metric & attributes + [InlineData("http/dup", true, true, 6, 5)] // emits both old & new + public async Task RequestMetricIsCaptured(string environmentVarValue, bool validateOldSemConv, bool validateNewSemConv, int expectedOldTagsCount, int expectedNewTagsCount) { try { @@ -98,8 +98,8 @@ public async Task RequestMetricIsCaptured(string environmentVarValue, bool valid 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", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); + AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); } if (validateNewSemConv) @@ -116,8 +116,8 @@ public async Task RequestMetricIsCaptured(string environmentVarValue, bool valid 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); + AssertMetricPoint(metricPoints[0], expectedRoute: "api/Values", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); + AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); } } finally From 9fb75e560903cf766191f2f1d6b1df65d685f96f Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 29 Aug 2023 14:43:39 -0700 Subject: [PATCH 11/40] refactor --- .../Implementation/HttpInMetricsListener.cs | 78 ++++++++++--------- 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index f64b21ac7e0..78ce38365e7 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -96,11 +96,11 @@ public override void OnEventWritten(string name, object payload) return; } - TagList oldTags = default, newTags = default; - // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md if (this.emitOldAttributes) { + TagList oldTags = default; + oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, context.Request.Scheme)); oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, context.Request.Method)); @@ -115,66 +115,72 @@ public override void OnEventWritten(string name, object payload) oldTags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostPort, context.Request.Host.Port)); } } + +#if NET6_0_OR_GREATER + var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; + if (!string.IsNullOrEmpty(route)) + { + if (this.emitOldAttributes) + { + oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); + } + } +#endif + if (this.options.Enrich != null) + { + try + { + this.options.Enrich(HttpServerDurationMetricName, context, ref oldTags); + } + catch (Exception ex) + { + AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); + } + } + + // 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, oldTags); } // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md if (this.emitNewAttributes) { + TagList newTags = default; + newTags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); newTags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, context.Request.Scheme)); newTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, context.Request.Method)); newTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); - } #if NET6_0_OR_GREATER - var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; - if (!string.IsNullOrEmpty(route)) - { - if (this.emitOldAttributes) - { - oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); - } - - if (this.emitNewAttributes) + var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; + if (!string.IsNullOrEmpty(route)) { newTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); } - } #endif - if (this.options.Enrich != null) - { - try + if (this.options.Enrich != null) { - if (this.emitOldAttributes) + try { - this.options.Enrich(HttpServerDurationMetricName, context, ref oldTags); + this.options.Enrich(HttpServerDurationMetricName, context, ref newTags); } - - if (this.emitNewAttributes) + catch (Exception ex) { - this.options.Enrich(HttpServerDurationMetricName, context, ref newTags); + AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); } } - catch (Exception ex) - { - AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); - } - } - // 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. - if (this.emitNewAttributes) - { + // 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. + // 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, newTags); } - - if (this.emitOldAttributes) - { - this.httpServerDuration.Record(Activity.Current.Duration.TotalMilliseconds, oldTags); - } } } } From 6c86a4a98a1143cd8931424ae274f22528814f50 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 29 Aug 2023 14:49:42 -0700 Subject: [PATCH 12/40] fix name in log --- .../Implementation/HttpInMetricsListener.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 78ce38365e7..f319c4a496f 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -169,7 +169,7 @@ public override void OnEventWritten(string name, object payload) } catch (Exception ex) { - AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); + AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); } } From 3bcefaeaec227f47ffeebb94e8907988a84e0ac2 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 29 Aug 2023 14:50:04 -0700 Subject: [PATCH 13/40] another fix --- .../Implementation/HttpInMetricsListener.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index f319c4a496f..5310a8b651d 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -165,7 +165,7 @@ public override void OnEventWritten(string name, object payload) { try { - this.options.Enrich(HttpServerDurationMetricName, context, ref newTags); + this.options.Enrich(HttpServerRequestDurationMetricName, context, ref newTags); } catch (Exception ex) { From e588453b897ff2dc7988f3d544b19c71aa18d8fb Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 29 Aug 2023 14:55:00 -0700 Subject: [PATCH 14/40] more fixes --- .../Implementation/HttpInMetricsListener.cs | 42 +++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 5310a8b651d..eeb43e6f20b 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -70,22 +70,50 @@ public override void OnEventWritten(string name, object payload) var context = payload as HttpContext; if (context == null) { - AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); + if (this.emitOldAttributes) + { + AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); + } + + if (this.emitNewAttributes) + { + AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName); + } return; } - try + if (this.emitOldAttributes) { - if (this.options.Filter?.Invoke(HttpServerDurationMetricName, context) == false) + try + { + if (this.options.Filter?.Invoke(HttpServerDurationMetricName, context) == false) + { + AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); + return; + } + } + catch (Exception ex) { - AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); + AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); return; } } - catch (Exception ex) + + if (this.emitNewAttributes) { - AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); - return; + try + { + if (this.options.Filter?.Invoke(HttpServerRequestDurationMetricName, context) == false) + { + AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName); + return; + } + } + catch (Exception ex) + { + AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); + return; + } } // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. From 88355cd5568c8b616fcacec8108d9b6a0645d998 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 29 Aug 2023 15:47:10 -0700 Subject: [PATCH 15/40] big refactor, split Old and New into two methods --- .../Implementation/HttpInMetricsListener.cs | 204 +++++++++--------- 1 file changed, 107 insertions(+), 97 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index eeb43e6f20b..2e1827e8487 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -64,56 +64,41 @@ internal HttpInMetricsListener(string name, Meter meter, AspNetCoreMetricsInstru } public override void OnEventWritten(string name, object payload) + { + if (this.emitOldAttributes) + { + this.OnEventWritten_Old(name, payload); + } + + if (this.emitNewAttributes) + { + this.OnEventWritten_New(name, payload); + } + } + + public void OnEventWritten_Old(string name, object payload) { if (name == OnStopEvent) { var context = payload as HttpContext; if (context == null) { - if (this.emitOldAttributes) - { - AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); - } - - if (this.emitNewAttributes) - { - AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName); - } + AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); return; } - if (this.emitOldAttributes) + try { - try + if (this.options.Filter?.Invoke(HttpServerDurationMetricName, context) == false) { - if (this.options.Filter?.Invoke(HttpServerDurationMetricName, context) == false) - { - AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); - return; - } - } - catch (Exception ex) - { - AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); + AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); return; } } - - if (this.emitNewAttributes) + catch (Exception ex) { - try - { - if (this.options.Filter?.Invoke(HttpServerRequestDurationMetricName, context) == false) - { - AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName); - return; - } - } - catch (Exception ex) - { - AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); - return; - } + AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); + return; } // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. @@ -124,91 +109,116 @@ public override void OnEventWritten(string name, object payload) return; } - // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md - if (this.emitOldAttributes) - { - TagList oldTags = default; + TagList tags = default; - oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); - oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, context.Request.Scheme)); - oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, context.Request.Method)); - oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, context.Request.Scheme)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, context.Request.Method)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); - if (context.Request.Host.HasValue) - { - oldTags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostName, context.Request.Host.Host)); + if (context.Request.Host.HasValue) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostName, context.Request.Host.Host)); - if (context.Request.Host.Port is not null && context.Request.Host.Port != 80 && context.Request.Host.Port != 443) - { - oldTags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostPort, context.Request.Host.Port)); - } + if (context.Request.Host.Port is not null && context.Request.Host.Port != 80 && context.Request.Host.Port != 443) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostPort, context.Request.Host.Port)); } + } #if NET6_0_OR_GREATER - var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; - if (!string.IsNullOrEmpty(route)) + var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; + if (!string.IsNullOrEmpty(route)) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); + } +#endif + if (this.options.Enrich != null) + { + try { - if (this.emitOldAttributes) - { - oldTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); - } + this.options.Enrich(HttpServerDurationMetricName, context, ref tags); } -#endif - if (this.options.Enrich != null) + catch (Exception ex) { - try - { - this.options.Enrich(HttpServerDurationMetricName, context, ref oldTags); - } - catch (Exception ex) - { - AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); - } + AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); } + } - // 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, oldTags); + // 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); + } + } + + public void OnEventWritten_New(string name, object payload) + { + if (name == OnStopEvent) + { + var context = payload as HttpContext; + if (context == null) + { + AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName); + return; } - // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md - if (this.emitNewAttributes) + try + { + if (this.options.Filter?.Invoke(HttpServerRequestDurationMetricName, context) == false) + { + AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName); + return; + } + } + catch (Exception ex) { - TagList newTags = default; + AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); + return; + } - newTags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); - newTags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, context.Request.Scheme)); - newTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, context.Request.Method)); - newTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); + // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. + // Below is just a temporary way of achieving this suppression for metrics (we should consider suppressing traces too). + // If we want to suppress activity from Prometheus then we should use SuppressInstrumentationScope. + if (context.Request.Path.HasValue && context.Request.Path.Value.Contains("metrics")) + { + return; + } + + TagList tags = default; + + // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); + tags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, context.Request.Scheme)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, context.Request.Method)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); #if NET6_0_OR_GREATER - var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; - if (!string.IsNullOrEmpty(route)) + var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; + if (!string.IsNullOrEmpty(route)) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); + } +#endif + if (this.options.Enrich != null) + { + try { - newTags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); + this.options.Enrich(HttpServerRequestDurationMetricName, context, ref tags); } -#endif - if (this.options.Enrich != null) + catch (Exception ex) { - try - { - this.options.Enrich(HttpServerRequestDurationMetricName, context, ref newTags); - } - catch (Exception ex) - { - AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); - } + AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); } + } - // 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. + // 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. - // 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, newTags); - } + // 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); } } } From 6a3f651d693eb6855af839aee457fc82b8f21738 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Wed, 30 Aug 2023 11:21:35 -0700 Subject: [PATCH 16/40] investigating test fix --- .../MetricTests.cs | 31 ++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index 57576c23ccc..82a9bb0d223 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -98,8 +98,10 @@ public async Task RequestMetricIsCaptured(string environmentVarValue, bool valid var metricPoints = GetMetricPoints(metric); Assert.Equal(2, metricPoints.Count); - AssertMetricPoint(metricPoints[0], expectedRoute: "api/Values", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); - AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); + // AssertMetricPoint(metricPoints[0], expectedRoute: "api/Values", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); + // AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); + AssertMetricPoint(GetMetricPoint(metricPoints, "api/Values").Value, expectedRoute: "api/Values", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); + AssertMetricPoint(GetMetricPoint(metricPoints, "api/Values/{id}").Value, expectedRoute: "api/Values/{id}", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); } if (validateNewSemConv) @@ -116,8 +118,10 @@ public async Task RequestMetricIsCaptured(string environmentVarValue, bool valid var metricPoints = GetMetricPoints(metric); Assert.Equal(2, metricPoints.Count); - AssertMetricPoint(metricPoints[0], expectedRoute: "api/Values", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); - AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); + // AssertMetricPoint(metricPoints[0], expectedRoute: "api/Values", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); + // AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); + AssertMetricPoint(GetMetricPoint(metricPoints, "api/Values").Value, expectedRoute: "api/Values", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); + AssertMetricPoint(GetMetricPoint(metricPoints, "api/Values/{id}").Value, expectedRoute: "api/Values/{id}", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); } } finally @@ -248,6 +252,25 @@ private static List GetMetricPoints(Metric metric) return metricPoints; } + private static MetricPoint? GetMetricPoint(List metricPoints, string expectedRoute) + { + for (int i = 0; i < metricPoints.Count; i++) + { + var metricPoint = metricPoints[i]; + + foreach (var tag in metricPoint.Tags) + { + if (tag.Key == SemanticConventions.AttributeHttpRoute && tag.Value.ToString() == expectedRoute) + { + return metricPoint; + } + } + } + + Assert.Fail("MetricPoint not found"); + return null; + } + private static KeyValuePair[] AssertMetricPoint( MetricPoint metricPoint, string expectedRoute = "api/Values", From 1a72356b318b9b5f968e9ed6b41eb859a698d882 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Wed, 30 Aug 2023 13:29:30 -0700 Subject: [PATCH 17/40] final fix for test issue --- .../MetricTests.cs | 51 ++++++++++++------- 1 file changed, 34 insertions(+), 17 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index 82a9bb0d223..d50fc5e5ead 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -98,10 +98,11 @@ public async Task RequestMetricIsCaptured(string environmentVarValue, bool valid var metricPoints = GetMetricPoints(metric); Assert.Equal(2, metricPoints.Count); - // AssertMetricPoint(metricPoints[0], expectedRoute: "api/Values", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); - // AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); - AssertMetricPoint(GetMetricPoint(metricPoints, "api/Values").Value, expectedRoute: "api/Values", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); - AssertMetricPoint(GetMetricPoint(metricPoints, "api/Values/{id}").Value, expectedRoute: "api/Values/{id}", expectedTagsCount: expectedOldTagsCount, validateOldSemConv: true); + AssertMetricPoints( + metricPoints: metricPoints, + expectedRoutes: new List { "api/Values", "api/Values/{id}" }, + expectedTagsCount: expectedOldTagsCount, + validateOldSemConv: true); } if (validateNewSemConv) @@ -118,10 +119,11 @@ public async Task RequestMetricIsCaptured(string environmentVarValue, bool valid var metricPoints = GetMetricPoints(metric); Assert.Equal(2, metricPoints.Count); - // AssertMetricPoint(metricPoints[0], expectedRoute: "api/Values", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); - // AssertMetricPoint(metricPoints[1], expectedRoute: "api/Values/{id}", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); - AssertMetricPoint(GetMetricPoint(metricPoints, "api/Values").Value, expectedRoute: "api/Values", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); - AssertMetricPoint(GetMetricPoint(metricPoints, "api/Values/{id}").Value, expectedRoute: "api/Values/{id}", expectedTagsCount: expectedNewTagsCount, validateNewSemConv: true); + AssertMetricPoints( + metricPoints: metricPoints, + expectedRoutes: new List { "api/Values", "api/Values/{id}" }, + expectedTagsCount: expectedNewTagsCount, + validateNewSemConv: true); } } finally @@ -252,23 +254,38 @@ private static List GetMetricPoints(Metric metric) return metricPoints; } - private static MetricPoint? GetMetricPoint(List metricPoints, string expectedRoute) + private static void AssertMetricPoints( + List metricPoints, + List expectedRoutes, + int expectedTagsCount, + bool validateNewSemConv = false, + bool validateOldSemConv = false) { - for (int i = 0; i < metricPoints.Count; i++) + // Assert that one MetricPoint exists for each ExpectedRoute + foreach (var expectedRoute in expectedRoutes) { - var metricPoint = metricPoints[i]; + MetricPoint? metricPoint = null; - foreach (var tag in metricPoint.Tags) + foreach (var mp in metricPoints) { - if (tag.Key == SemanticConventions.AttributeHttpRoute && tag.Value.ToString() == expectedRoute) + foreach (var tag in mp.Tags) { - return metricPoint; + if (tag.Key == SemanticConventions.AttributeHttpRoute && tag.Value.ToString() == expectedRoute) + { + metricPoint = mp; + } } } - } - Assert.Fail("MetricPoint not found"); - return null; + if (metricPoint.HasValue) + { + AssertMetricPoint(metricPoint.Value, expectedRoute, expectedTagsCount, validateNewSemConv, validateOldSemConv); + } + else + { + Assert.Fail($"A metric for route '{expectedRoute}' was not found"); + } + } } private static KeyValuePair[] AssertMetricPoint( From b57320de7dee05c5a362e00bec7e7afe4d6f2de2 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 5 Sep 2023 16:48:06 -0700 Subject: [PATCH 18/40] change unit to seconds --- .../Implementation/HttpInMetricsListener.cs | 9 ++------ .../MetricTests.cs | 22 ++++++++++++------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 2e1827e8487..700d844ee4b 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -57,9 +57,7 @@ internal HttpInMetricsListener(string name, Meter meter, AspNetCoreMetricsInstru if (this.emitNewAttributes) { - // 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(HttpServerRequestDurationMetricName, "ms", "Measures the duration of inbound HTTP requests."); + this.httpServerRequestDuration = meter.CreateHistogram(HttpServerRequestDurationMetricName, "s", "Measures the duration of inbound HTTP requests."); } } @@ -215,10 +213,7 @@ public void OnEventWritten_New(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. - - // 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); + this.httpServerRequestDuration.Record(Activity.Current.Duration.TotalSeconds, tags); } } } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index d50fc5e5ead..1e73d14d65b 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -113,9 +113,7 @@ public async Task RequestMetricIsCaptured(string environmentVarValue, bool valid 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); + Assert.Equal("s", metric.Unit); var metricPoints = GetMetricPoints(metric); Assert.Equal(2, metricPoints.Count); @@ -349,11 +347,19 @@ private static KeyValuePair[] AssertMetricPoint( 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 { 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000, double.PositiveInfinity }, - actual: histogramBounds); + if (validateNewSemConv) + { + Assert.Equal( + expected: new List { 0, 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); + } + + if (validateOldSemConv) + { + Assert.Equal( + expected: new List { 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000, double.PositiveInfinity }, + actual: histogramBounds); + } return attributes; } From 7fdc29fa14c8e7aee344f15cb7e9c2a646d6b68b Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 5 Sep 2023 16:48:29 -0700 Subject: [PATCH 19/40] update Readme --- src/OpenTelemetry.Instrumentation.AspNetCore/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index 6f9f3324834..e9ffa48d6c1 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -93,13 +93,13 @@ public void ConfigureServices(IServiceCollection services) The instrumentation is implemented based on [metrics semantic conventions](https://github.com/open-telemetry/semantic-conventions/blob/4bbb8c907402caa90bc077214e8a2c78807c1ab9/docs/http/http-metrics.md). -Have you opt-ed into the new Http Semantic Conventions? +Have you opt-ed into the new Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`? * 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. | + | `http.server.request.duration` | Histogram | `s` | Measures the duration of inbound HTTP requests. | * If no, the instrumentation supports the following metric. From a782b514a0649ad75246c1aa921437db1bba0318 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 7 Sep 2023 11:17:01 -0700 Subject: [PATCH 20/40] cleanup if condition --- .../Implementation/HttpInMetricsListener.cs | 197 +++++++++--------- 1 file changed, 97 insertions(+), 100 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 700d844ee4b..e9f4576a8c1 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -63,66 +63,67 @@ internal HttpInMetricsListener(string name, Meter meter, AspNetCoreMetricsInstru public override void OnEventWritten(string name, object payload) { - if (this.emitOldAttributes) + if (name == OnStopEvent) { - this.OnEventWritten_Old(name, payload); - } + if (this.emitOldAttributes) + { + this.OnEventWritten_Old(name, payload); + } - if (this.emitNewAttributes) - { - this.OnEventWritten_New(name, payload); + if (this.emitNewAttributes) + { + this.OnEventWritten_New(name, payload); + } } } public void OnEventWritten_Old(string name, object payload) { - if (name == OnStopEvent) + var context = payload as HttpContext; + if (context == null) { - var context = payload as HttpContext; - if (context == null) - { - AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); - return; - } + AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); + return; + } - try - { - if (this.options.Filter?.Invoke(HttpServerDurationMetricName, context) == false) - { - AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); - return; - } - } - catch (Exception ex) + try + { + if (this.options.Filter?.Invoke(HttpServerDurationMetricName, context) == false) { - AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); + AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName); return; } + } + catch (Exception ex) + { + AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); + return; + } - // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. - // Below is just a temporary way of achieving this suppression for metrics (we should consider suppressing traces too). - // If we want to suppress activity from Prometheus then we should use SuppressInstrumentationScope. - if (context.Request.Path.HasValue && context.Request.Path.Value.Contains("metrics")) - { - return; - } + // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. + // Below is just a temporary way of achieving this suppression for metrics (we should consider suppressing traces too). + // If we want to suppress activity from Prometheus then we should use SuppressInstrumentationScope. + if (context.Request.Path.HasValue && context.Request.Path.Value.Contains("metrics")) + { + return; + } - TagList tags = default; + TagList tags = default; - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, context.Request.Scheme)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, context.Request.Method)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, context.Request.Scheme)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, context.Request.Method)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); - if (context.Request.Host.HasValue) - { - tags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostName, context.Request.Host.Host)); + if (context.Request.Host.HasValue) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostName, 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(SemanticConventions.AttributeNetHostPort, context.Request.Host.Port)); - } + if (context.Request.Host.Port is not null && context.Request.Host.Port != 80 && context.Request.Host.Port != 443) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostPort, context.Request.Host.Port)); } + } #if NET6_0_OR_GREATER var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; @@ -131,65 +132,62 @@ public void OnEventWritten_Old(string name, object payload) tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); } #endif - if (this.options.Enrich != null) + if (this.options.Enrich != null) + { + try { - try - { - this.options.Enrich(HttpServerDurationMetricName, context, ref tags); - } - catch (Exception ex) - { - AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); - } + this.options.Enrich(HttpServerDurationMetricName, context, ref tags); + } + catch (Exception ex) + { + AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName, ex); } - - // 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); } + + // 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); } public void OnEventWritten_New(string name, object payload) { - if (name == OnStopEvent) + var context = payload as HttpContext; + if (context == null) { - var context = payload as HttpContext; - if (context == null) - { - AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName); - return; - } + AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName); + return; + } - try - { - if (this.options.Filter?.Invoke(HttpServerRequestDurationMetricName, context) == false) - { - AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName); - return; - } - } - catch (Exception ex) + try + { + if (this.options.Filter?.Invoke(HttpServerRequestDurationMetricName, context) == false) { - AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); + AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName); return; } + } + catch (Exception ex) + { + AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); + return; + } - // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. - // Below is just a temporary way of achieving this suppression for metrics (we should consider suppressing traces too). - // If we want to suppress activity from Prometheus then we should use SuppressInstrumentationScope. - if (context.Request.Path.HasValue && context.Request.Path.Value.Contains("metrics")) - { - return; - } + // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. + // Below is just a temporary way of achieving this suppression for metrics (we should consider suppressing traces too). + // If we want to suppress activity from Prometheus then we should use SuppressInstrumentationScope. + if (context.Request.Path.HasValue && context.Request.Path.Value.Contains("metrics")) + { + return; + } - TagList tags = default; + TagList tags = default; - // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md - tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); - tags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, context.Request.Scheme)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, context.Request.Method)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); + // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); + tags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, context.Request.Scheme)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, context.Request.Method)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); #if NET6_0_OR_GREATER var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; @@ -198,22 +196,21 @@ public void OnEventWritten_New(string name, object payload) tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); } #endif - if (this.options.Enrich != null) + if (this.options.Enrich != null) + { + try { - try - { - this.options.Enrich(HttpServerRequestDurationMetricName, context, ref tags); - } - catch (Exception ex) - { - AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); - } + this.options.Enrich(HttpServerRequestDurationMetricName, context, ref tags); + } + catch (Exception ex) + { + AspNetCoreInstrumentationEventSource.Log.EnrichmentException(nameof(HttpInMetricsListener), EventName, HttpServerRequestDurationMetricName, ex); } - - // 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.httpServerRequestDuration.Record(Activity.Current.Duration.TotalSeconds, tags); } + + // 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.httpServerRequestDuration.Record(Activity.Current.Duration.TotalSeconds, tags); } } From 3788d1536a873acbb7c410072aa79c7e78775686 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 7 Sep 2023 11:28:03 -0700 Subject: [PATCH 21/40] fix indentation --- .../Implementation/HttpInMetricsListener.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index e9f4576a8c1..9a8b35040c6 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -126,11 +126,11 @@ public void OnEventWritten_Old(string name, object payload) } #if NET6_0_OR_GREATER - var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; - if (!string.IsNullOrEmpty(route)) - { - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); - } + var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; + if (!string.IsNullOrEmpty(route)) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); + } #endif if (this.options.Enrich != null) { @@ -190,11 +190,11 @@ public void OnEventWritten_New(string name, object payload) tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); #if NET6_0_OR_GREATER - var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; - if (!string.IsNullOrEmpty(route)) - { - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); - } + var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; + if (!string.IsNullOrEmpty(route)) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); + } #endif if (this.options.Enrich != null) { From 275de4ea7eeed6e1244d875a50362b5d57ba39a1 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 7 Sep 2023 13:40:48 -0700 Subject: [PATCH 22/40] update readme --- src/OpenTelemetry.Instrumentation.AspNetCore/README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index e9ffa48d6c1..37e6dc55725 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -101,6 +101,12 @@ Have you opt-ed into the new Http Semantic Conventions using `OTEL_SEMCONV_STABI |-------|-----------------|------|-------------| | `http.server.request.duration` | Histogram | `s` | Measures the duration of inbound HTTP requests. | + This metric is emitted in "seconds" as per the semantic convention. While the convention + [recommends using custom histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration) + , this feature is not yet available in OTel .NET. However, the OTel SDK includes a + [workaround](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) + using the recommended buckets. + * If no, the instrumentation supports the following metric. | Name | Instrument Type | Unit | Description | From c6d5d761fa2eafffc675947e1ee7b9728eb54a83 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 7 Sep 2023 13:49:28 -0700 Subject: [PATCH 23/40] change test to use IConfiguration --- .../MetricTests.cs | 117 +++++++++--------- 1 file changed, 57 insertions(+), 60 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index 1e73d14d65b..93a677492e6 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -19,6 +19,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using OpenTelemetry.Metrics; @@ -55,78 +56,74 @@ public void AddAspNetCoreInstrumentation_BadArgs() [InlineData("http/dup", true, true, 6, 5)] // emits both old & new public async Task RequestMetricIsCaptured(string environmentVarValue, bool validateOldSemConv, bool validateNewSemConv, int expectedOldTagsCount, int expectedNewTagsCount) { - try - { - Environment.SetEnvironmentVariable(SemanticConventionOptInKeyName, environmentVarValue); + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { [SemanticConventionOptInKeyName] = environmentVarValue }) + .Build(); - var metricItems = new List(); + var metricItems = new List(); - this.meterProvider = Sdk.CreateMeterProviderBuilder() - .AddAspNetCoreInstrumentation() - .AddInMemoryExporter(metricItems) - .Build(); + this.meterProvider = Sdk.CreateMeterProviderBuilder() + .ConfigureServices(services => services.AddSingleton(configuration)) + .AddAspNetCoreInstrumentation() + .AddInMemoryExporter(metricItems) + .Build(); - using (var client = this.factory - .WithWebHostBuilder(builder => - { - builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); - }) - .CreateClient()) + using (var client = this.factory + .WithWebHostBuilder(builder => { - using var response1 = await client.GetAsync("/api/values").ConfigureAwait(false); - using var response2 = await client.GetAsync("/api/values/2").ConfigureAwait(false); + 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); - 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(); - if (validateOldSemConv) - { - var requestMetrics = metricItems - .Where(item => item.Name == "http.server.duration") - .ToArray(); - - var metric = Assert.Single(requestMetrics); - Assert.Equal("ms", metric.Unit); - var metricPoints = GetMetricPoints(metric); - Assert.Equal(2, metricPoints.Count); - - AssertMetricPoints( - metricPoints: metricPoints, - expectedRoutes: new List { "api/Values", "api/Values/{id}" }, - expectedTagsCount: expectedOldTagsCount, - validateOldSemConv: true); - } + if (validateOldSemConv) + { + var requestMetrics = metricItems + .Where(item => item.Name == "http.server.duration") + .ToArray(); + + var metric = Assert.Single(requestMetrics); + Assert.Equal("ms", metric.Unit); + var metricPoints = GetMetricPoints(metric); + Assert.Equal(2, metricPoints.Count); + + AssertMetricPoints( + metricPoints: metricPoints, + expectedRoutes: new List { "api/Values", "api/Values/{id}" }, + expectedTagsCount: expectedOldTagsCount, + validateOldSemConv: true); + } - if (validateNewSemConv) - { - var requestMetrics = metricItems - .Where(item => item.Name == "http.server.request.duration") - .ToArray(); + if (validateNewSemConv) + { + var requestMetrics = metricItems + .Where(item => item.Name == "http.server.request.duration") + .ToArray(); - var metric = Assert.Single(requestMetrics); + var metric = Assert.Single(requestMetrics); - Assert.Equal("s", metric.Unit); - var metricPoints = GetMetricPoints(metric); - Assert.Equal(2, metricPoints.Count); + Assert.Equal("s", metric.Unit); + var metricPoints = GetMetricPoints(metric); + Assert.Equal(2, metricPoints.Count); - AssertMetricPoints( - metricPoints: metricPoints, - expectedRoutes: new List { "api/Values", "api/Values/{id}" }, - expectedTagsCount: expectedNewTagsCount, - validateNewSemConv: true); - } - } - finally - { - Environment.SetEnvironmentVariable(SemanticConventionOptInKeyName, null); + AssertMetricPoints( + metricPoints: metricPoints, + expectedRoutes: new List { "api/Values", "api/Values/{id}" }, + expectedTagsCount: expectedNewTagsCount, + validateNewSemConv: true); } } From 3789fdd0f40b853be4e9107c1b69fb006c4a3ed3 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Fri, 8 Sep 2023 15:37:51 -0700 Subject: [PATCH 24/40] markdown lint --- src/OpenTelemetry.Instrumentation.AspNetCore/README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index 37e6dc55725..3694890f002 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -101,10 +101,10 @@ Have you opt-ed into the new Http Semantic Conventions using `OTEL_SEMCONV_STABI |-------|-----------------|------|-------------| | `http.server.request.duration` | Histogram | `s` | Measures the duration of inbound HTTP requests. | - This metric is emitted in "seconds" as per the semantic convention. While the convention - [recommends using custom histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration) - , this feature is not yet available in OTel .NET. However, the OTel SDK includes a - [workaround](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) + This metric is emitted in "seconds" as per the semantic convention. While + the convention [recommends using custom histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration) + , this feature is not yet available in OTel .NET. However, the OTel SDK + includes a [workaround](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) using the recommended buckets. * If no, the instrumentation supports the following metric. From f110b29d03c60dbf173c5207b39582bc26c796db Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Fri, 8 Sep 2023 17:22:40 -0700 Subject: [PATCH 25/40] split test methods --- .../MetricTests.cs | 168 ++++++++++++++---- 1 file changed, 133 insertions(+), 35 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index 93a677492e6..f0bb01f2f6b 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -50,14 +50,11 @@ public void AddAspNetCoreInstrumentation_BadArgs() Assert.Throws(() => builder.AddAspNetCoreInstrumentation()); } - [Theory] - [InlineData(null, true, false, 6, 0)] // emits old metric & attributes - [InlineData("http", false, true, 0, 5)] // emits new metric & attributes - [InlineData("http/dup", true, true, 6, 5)] // emits both old & new - public async Task RequestMetricIsCaptured(string environmentVarValue, bool validateOldSemConv, bool validateNewSemConv, int expectedOldTagsCount, int expectedNewTagsCount) + [Fact] + public async Task RequestMetricIsCaptured_Old() { var configuration = new ConfigurationBuilder() - .AddInMemoryCollection(new Dictionary { [SemanticConventionOptInKeyName] = environmentVarValue }) + .AddInMemoryCollection(new Dictionary { [SemanticConventionOptInKeyName] = null }) .Build(); var metricItems = new List(); @@ -89,42 +86,143 @@ public async Task RequestMetricIsCaptured(string environmentVarValue, bool valid this.meterProvider.Dispose(); - if (validateOldSemConv) + var requestMetrics = metricItems + .Where(item => item.Name == "http.server.duration") + .ToArray(); + + var metric = Assert.Single(requestMetrics); + Assert.Equal("ms", metric.Unit); + var metricPoints = GetMetricPoints(metric); + Assert.Equal(2, metricPoints.Count); + + AssertMetricPoints( + metricPoints: metricPoints, + expectedRoutes: new List { "api/Values", "api/Values/{id}" }, + expectedTagsCount: 6, + validateOldSemConv: true); + } + + [Fact] + public async Task RequestMetricIsCaptured_New() + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { [SemanticConventionOptInKeyName] = "http" }) + .Build(); + + var metricItems = new List(); + + this.meterProvider = Sdk.CreateMeterProviderBuilder() + .ConfigureServices(services => services.AddSingleton(configuration)) + .AddAspNetCoreInstrumentation() + .AddInMemoryExporter(metricItems) + .Build(); + + using (var client = this.factory + .WithWebHostBuilder(builder => + { + builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); + }) + .CreateClient()) { - var requestMetrics = metricItems - .Where(item => item.Name == "http.server.duration") - .ToArray(); - - var metric = Assert.Single(requestMetrics); - Assert.Equal("ms", metric.Unit); - var metricPoints = GetMetricPoints(metric); - Assert.Equal(2, metricPoints.Count); - - AssertMetricPoints( - metricPoints: metricPoints, - expectedRoutes: new List { "api/Values", "api/Values/{id}" }, - expectedTagsCount: expectedOldTagsCount, - validateOldSemConv: true); + 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(); } - if (validateNewSemConv) - { - var requestMetrics = metricItems - .Where(item => item.Name == "http.server.request.duration") - .ToArray(); + // 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); - var metric = Assert.Single(requestMetrics); + this.meterProvider.Dispose(); + + var requestMetrics = metricItems + .Where(item => item.Name == "http.server.request.duration") + .ToArray(); + + var metric = Assert.Single(requestMetrics); - Assert.Equal("s", metric.Unit); - var metricPoints = GetMetricPoints(metric); - Assert.Equal(2, metricPoints.Count); + Assert.Equal("s", metric.Unit); + var metricPoints = GetMetricPoints(metric); + Assert.Equal(2, metricPoints.Count); - AssertMetricPoints( - metricPoints: metricPoints, - expectedRoutes: new List { "api/Values", "api/Values/{id}" }, - expectedTagsCount: expectedNewTagsCount, - validateNewSemConv: true); + AssertMetricPoints( + metricPoints: metricPoints, + expectedRoutes: new List { "api/Values", "api/Values/{id}" }, + expectedTagsCount: 5, + validateNewSemConv: true); + } + + [Fact] + public async Task RequestMetricIsCaptured_Dup() + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { [SemanticConventionOptInKeyName] = "http/dup" }) + .Build(); + + var metricItems = new List(); + + this.meterProvider = Sdk.CreateMeterProviderBuilder() + .ConfigureServices(services => services.AddSingleton(configuration)) + .AddAspNetCoreInstrumentation() + .AddInMemoryExporter(metricItems) + .Build(); + + using (var client = this.factory + .WithWebHostBuilder(builder => + { + 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); + + 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); + + this.meterProvider.Dispose(); + + // Validate Old Semantic Convention + var requestMetrics = metricItems + .Where(item => item.Name == "http.server.duration") + .ToArray(); + + var metric = Assert.Single(requestMetrics); + Assert.Equal("ms", metric.Unit); + var metricPoints = GetMetricPoints(metric); + Assert.Equal(2, metricPoints.Count); + + AssertMetricPoints( + metricPoints: metricPoints, + expectedRoutes: new List { "api/Values", "api/Values/{id}" }, + expectedTagsCount: 6, + validateOldSemConv: true); + + // Validate New Semantic Convention + requestMetrics = metricItems + .Where(item => item.Name == "http.server.request.duration") + .ToArray(); + + metric = Assert.Single(requestMetrics); + + Assert.Equal("s", metric.Unit); + metricPoints = GetMetricPoints(metric); + Assert.Equal(2, metricPoints.Count); + + AssertMetricPoints( + metricPoints: metricPoints, + expectedRoutes: new List { "api/Values", "api/Values/{id}" }, + expectedTagsCount: 5, + validateNewSemConv: true); } [Fact] From 7c11aac9b1492a595b4847a84cf797b5e9a89670 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Date: Mon, 11 Sep 2023 12:53:23 -0700 Subject: [PATCH 26/40] Update src/OpenTelemetry.Instrumentation.AspNetCore/README.md Co-authored-by: Vishwesh Bankwar --- src/OpenTelemetry.Instrumentation.AspNetCore/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index 3694890f002..772df87394a 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -103,7 +103,7 @@ Have you opt-ed into the new Http Semantic Conventions using `OTEL_SEMCONV_STABI This metric is emitted in "seconds" as per the semantic convention. While the convention [recommends using custom histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration) - , this feature is not yet available in OTel .NET. However, the OTel SDK + , this feature is not yet available via .NET metrics API. A workaround has been included in OTel SDK starting version `1.6.0` which applies recommended buckets by default for this metric. includes a [workaround](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) using the recommended buckets. From 9ab958c68aa15fa6f62b706a7dfd49f1ab9c1221 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Mon, 11 Sep 2023 17:43:20 -0700 Subject: [PATCH 27/40] cleanup Readme after suggestion conflict --- src/OpenTelemetry.Instrumentation.AspNetCore/README.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index 772df87394a..f9fcd66be12 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -103,9 +103,10 @@ Have you opt-ed into the new Http Semantic Conventions using `OTEL_SEMCONV_STABI This metric is emitted in "seconds" as per the semantic convention. While the convention [recommends using custom histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration) - , this feature is not yet available via .NET metrics API. A workaround has been included in OTel SDK starting version `1.6.0` which applies recommended buckets by default for this metric. - includes a [workaround](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) - using the recommended buckets. + , this feature is not yet available via .NET Metrics API. + A [workaround](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) + has been included in OTel SDK starting version `1.6.0` which applies + recommended buckets by default for this metric. * If no, the instrumentation supports the following metric. From 3646823c10af0eabaf16ddce49ebf25a61ae97df Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Mon, 11 Sep 2023 17:44:15 -0700 Subject: [PATCH 28/40] update changelog --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 6316d096e0e..b2d3de29003 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,7 +2,10 @@ ## Unreleased -* TODO: UPDATE THIS +* Added new metric `http.server.request.duration` which replaces + `http.server.duration` if a user opted into the new semantic convention + by setting the `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable. + This new metric is emitted in seconds and has unique histogram buckets. ## 1.5.1-beta.1 From 314c9d9b75e230a350dbf22a84d5b2212034d002 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 12 Sep 2023 13:22:23 -0700 Subject: [PATCH 29/40] update changelog --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index b2d3de29003..93537f40496 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -5,7 +5,8 @@ * Added new metric `http.server.request.duration` which replaces `http.server.duration` if a user opted into the new semantic convention by setting the `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable. - This new metric is emitted in seconds and has unique histogram buckets. + This new metric is emitted as seconds and has unique + [histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration). ## 1.5.1-beta.1 From 255b2dae3396add69687fd025afe28f128d668bd Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 12 Sep 2023 13:39:00 -0700 Subject: [PATCH 30/40] refactor tests --- .../MetricTests.cs | 164 +++++++++++------- 1 file changed, 104 insertions(+), 60 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index f0bb01f2f6b..8b179278a49 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -95,11 +95,10 @@ public async Task RequestMetricIsCaptured_Old() var metricPoints = GetMetricPoints(metric); Assert.Equal(2, metricPoints.Count); - AssertMetricPoints( + AssertMetricPoints_Old( metricPoints: metricPoints, expectedRoutes: new List { "api/Values", "api/Values/{id}" }, - expectedTagsCount: 6, - validateOldSemConv: true); + expectedTagsCount: 6); } [Fact] @@ -148,11 +147,10 @@ public async Task RequestMetricIsCaptured_New() var metricPoints = GetMetricPoints(metric); Assert.Equal(2, metricPoints.Count); - AssertMetricPoints( + AssertMetricPoints_New( metricPoints: metricPoints, expectedRoutes: new List { "api/Values", "api/Values/{id}" }, - expectedTagsCount: 5, - validateNewSemConv: true); + expectedTagsCount: 5); } [Fact] @@ -201,11 +199,10 @@ public async Task RequestMetricIsCaptured_Dup() var metricPoints = GetMetricPoints(metric); Assert.Equal(2, metricPoints.Count); - AssertMetricPoints( + AssertMetricPoints_Old( metricPoints: metricPoints, expectedRoutes: new List { "api/Values", "api/Values/{id}" }, - expectedTagsCount: 6, - validateOldSemConv: true); + expectedTagsCount: 6); // Validate New Semantic Convention requestMetrics = metricItems @@ -218,11 +215,10 @@ public async Task RequestMetricIsCaptured_Dup() metricPoints = GetMetricPoints(metric); Assert.Equal(2, metricPoints.Count); - AssertMetricPoints( + AssertMetricPoints_New( metricPoints: metricPoints, expectedRoutes: new List { "api/Values", "api/Values/{id}" }, - expectedTagsCount: 5, - validateNewSemConv: true); + expectedTagsCount: 5); } [Fact] @@ -268,7 +264,7 @@ void ConfigureTestServices(IServiceCollection services) // Assert single because we filtered out one route var metricPoint = Assert.Single(GetMetricPoints(metric)); - AssertMetricPoint(metricPoint, validateOldSemConv: true); + AssertMetricPoint_Old(metricPoint); } [Fact] @@ -322,7 +318,7 @@ void ConfigureTestServices(IServiceCollection services) var metric = Assert.Single(requestMetrics); var metricPoint = Assert.Single(GetMetricPoints(metric)); - var tags = AssertMetricPoint(metricPoint, expectedTagsCount: StandardTagsCount + 2, validateOldSemConv: true); + var tags = AssertMetricPoint_Old(metricPoint, expectedTagsCount: StandardTagsCount + 2); Assert.Contains(tagsToAdd[0], tags); Assert.Contains(tagsToAdd[1], tags); @@ -347,12 +343,10 @@ private static List GetMetricPoints(Metric metric) return metricPoints; } - private static void AssertMetricPoints( + private static void AssertMetricPoints_New( List metricPoints, List expectedRoutes, - int expectedTagsCount, - bool validateNewSemConv = false, - bool validateOldSemConv = false) + int expectedTagsCount) { // Assert that one MetricPoint exists for each ExpectedRoute foreach (var expectedRoute in expectedRoutes) @@ -372,7 +366,7 @@ private static void AssertMetricPoints( if (metricPoint.HasValue) { - AssertMetricPoint(metricPoint.Value, expectedRoute, expectedTagsCount, validateNewSemConv, validateOldSemConv); + AssertMetricPoint_New(metricPoint.Value, expectedRoute, expectedTagsCount); } else { @@ -381,12 +375,42 @@ private static void AssertMetricPoints( } } - private static KeyValuePair[] AssertMetricPoint( + private static void AssertMetricPoints_Old( + List metricPoints, + List expectedRoutes, + int expectedTagsCount) + { + // Assert that one MetricPoint exists for each ExpectedRoute + foreach (var expectedRoute in expectedRoutes) + { + MetricPoint? metricPoint = null; + + foreach (var mp in metricPoints) + { + foreach (var tag in mp.Tags) + { + if (tag.Key == SemanticConventions.AttributeHttpRoute && tag.Value.ToString() == expectedRoute) + { + metricPoint = mp; + } + } + } + + if (metricPoint.HasValue) + { + AssertMetricPoint_Old(metricPoint.Value, expectedRoute, expectedTagsCount); + } + else + { + Assert.Fail($"A metric for route '{expectedRoute}' was not found"); + } + } + } + + private static KeyValuePair[] AssertMetricPoint_New( MetricPoint metricPoint, string expectedRoute = "api/Values", - int expectedTagsCount = StandardTagsCount, - bool validateNewSemConv = false, - bool validateOldSemConv = false) + int expectedTagsCount = StandardTagsCount) { var count = metricPoint.GetHistogramCount(); var sum = metricPoint.GetHistogramSum(); @@ -404,35 +428,16 @@ private static KeyValuePair[] AssertMetricPoint( // Inspect Attributes Assert.Equal(expectedTagsCount, attributes.Length); - if (validateNewSemConv) - { - var method = new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, "GET"); - var scheme = new KeyValuePair(SemanticConventions.AttributeUrlScheme, "http"); - var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, 200); - var flavor = new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, "1.1"); - var route = new KeyValuePair(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(SemanticConventions.AttributeHttpMethod, "GET"); - var scheme = new KeyValuePair(SemanticConventions.AttributeHttpScheme, "http"); - var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, 200); - var flavor = new KeyValuePair(SemanticConventions.AttributeHttpFlavor, "1.1"); - var host = new KeyValuePair(SemanticConventions.AttributeNetHostName, "localhost"); - var route = new KeyValuePair(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); - } + var method = new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, "GET"); + var scheme = new KeyValuePair(SemanticConventions.AttributeUrlScheme, "http"); + var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, 200); + var flavor = new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, "1.1"); + var route = new KeyValuePair(SemanticConventions.AttributeHttpRoute, expectedRoute); + Assert.Contains(method, attributes); + Assert.Contains(scheme, attributes); + Assert.Contains(statusCode, attributes); + Assert.Contains(flavor, attributes); + Assert.Contains(route, attributes); // Inspect Histogram Bounds var histogramBuckets = metricPoint.GetHistogramBuckets(); @@ -442,20 +447,59 @@ private static KeyValuePair[] AssertMetricPoint( histogramBounds.Add(t.ExplicitBound); } - if (validateNewSemConv) + Assert.Equal( + expected: new List { 0, 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); + + return attributes; + } + + private static KeyValuePair[] AssertMetricPoint_Old( + MetricPoint metricPoint, + string expectedRoute = "api/Values", + int expectedTagsCount = StandardTagsCount) + { + var count = metricPoint.GetHistogramCount(); + var sum = metricPoint.GetHistogramSum(); + + Assert.Equal(1L, count); + Assert.True(sum > 0); + + var attributes = new KeyValuePair[metricPoint.Tags.Count]; + int i = 0; + foreach (var tag in metricPoint.Tags) { - Assert.Equal( - expected: new List { 0, 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); + attributes[i++] = tag; } - if (validateOldSemConv) + // Inspect Attributes + Assert.Equal(expectedTagsCount, attributes.Length); + + var method = new KeyValuePair(SemanticConventions.AttributeHttpMethod, "GET"); + var scheme = new KeyValuePair(SemanticConventions.AttributeHttpScheme, "http"); + var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, 200); + var flavor = new KeyValuePair(SemanticConventions.AttributeHttpFlavor, "1.1"); + var host = new KeyValuePair(SemanticConventions.AttributeNetHostName, "localhost"); + var route = new KeyValuePair(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(); + foreach (var t in histogramBuckets) { - Assert.Equal( - expected: new List { 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000, double.PositiveInfinity }, - actual: histogramBounds); + histogramBounds.Add(t.ExplicitBound); } + Assert.Equal( + expected: new List { 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000, double.PositiveInfinity }, + actual: histogramBounds); + return attributes; } } From 12d10cfcb06dcb9e323d290c7a5df92ff87109c1 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Date: Tue, 12 Sep 2023 14:07:09 -0700 Subject: [PATCH 31/40] Update src/OpenTelemetry.Instrumentation.AspNetCore/README.md Co-authored-by: Vishwesh Bankwar --- src/OpenTelemetry.Instrumentation.AspNetCore/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index f9fcd66be12..0f560fb9b89 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -101,7 +101,7 @@ Have you opt-ed into the new Http Semantic Conventions using `OTEL_SEMCONV_STABI |-------|-----------------|------|-------------| | `http.server.request.duration` | Histogram | `s` | Measures the duration of inbound HTTP requests. | - This metric is emitted in "seconds" as per the semantic convention. While + This metric is emitted in `seconds` as per the semantic convention. While the convention [recommends using custom histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration) , this feature is not yet available via .NET Metrics API. A [workaround](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) From c38a93e28298071782f2995ef4aef697b1f5040e Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 12 Sep 2023 15:11:05 -0700 Subject: [PATCH 32/40] update changelog --- .../CHANGELOG.md | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 93537f40496..e247daa3ebd 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,11 +2,15 @@ ## Unreleased -* Added new metric `http.server.request.duration` which replaces - `http.server.duration` if a user opted into the new semantic convention - by setting the `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable. - This new metric is emitted as seconds and has unique - [histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration). +* Introduced a new metric, `http.server.request.duration`, for users who opt + into the new semantic convention by configuring the `OTEL_SEMCONV_STABILITY_OPT_IN` + environment variable. This metric measures time in seconds and offers unique + histogram buckets as per the + [spec](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration) + , replacing the previous `http.server.duration metric`, which measured time in + milliseconds. + * Former buckets: `0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000` + * New buckets: `0, 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` ## 1.5.1-beta.1 From 1f787be96616b3112241442fba843bd793b27edc Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 12 Sep 2023 15:29:00 -0700 Subject: [PATCH 33/40] markdownlint --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index e247daa3ebd..02f48c52e57 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -9,8 +9,10 @@ [spec](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration) , replacing the previous `http.server.duration metric`, which measured time in milliseconds. - * Former buckets: `0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000` - * New buckets: `0, 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` + * Former buckets: `0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, + 5000, 7500, 10000` + * New buckets: `0, 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` ## 1.5.1-beta.1 From 0b257af2891e2487292b35c3cf03401d49129b86 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Wed, 13 Sep 2023 15:21:14 -0700 Subject: [PATCH 34/40] rewrite changelog --- .../CHANGELOG.md | 30 ++++++++++++------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 02f48c52e57..8aa187f1fa3 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,17 +2,25 @@ ## Unreleased -* Introduced a new metric, `http.server.request.duration`, for users who opt - into the new semantic convention by configuring the `OTEL_SEMCONV_STABILITY_OPT_IN` - environment variable. This metric measures time in seconds and offers unique - histogram buckets as per the - [spec](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration) - , replacing the previous `http.server.duration metric`, which measured time in - milliseconds. - * Former buckets: `0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, - 5000, 7500, 10000` - * New buckets: `0, 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` +* Introduced a new metric, `http.server.request.duration`, for users who opt-in + to the new semantic convention by configuring the `OTEL_SEMCONV_STABILITY_OPT_IN` + environment variable. This metric measures time in seconds and uses unique + histogram buckets as defined by the semantic conventions for Http + [metrics](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md) + While the convention recommends using custom histogram buckets, this feature + is not yet available via .NET Metrics API. A + [workaround](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) + has been included in OTel SDK starting version `1.6.0` which applies + recommended buckets by default for this metric. + ([#4802](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4802)) + * New metric: `http.server.request.duration` + * Unit: `seconds` + * Histogram Buckets: `0, 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` + * Replaces old metric: `http.server.duration` + * Unit: `miliseconds` + * Histogram Buckets: `0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, + 5000, 7500, 10000` ## 1.5.1-beta.1 From 49875401e8ad8d0c1c7e49d52efef7b080bd573c Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Wed, 13 Sep 2023 15:27:02 -0700 Subject: [PATCH 35/40] rewrite Readme --- .../README.md | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index 0f560fb9b89..856417bb242 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -91,28 +91,30 @@ public void ConfigureServices(IServiceCollection services) #### List of metrics produced The instrumentation is implemented based on -[metrics semantic conventions](https://github.com/open-telemetry/semantic-conventions/blob/4bbb8c907402caa90bc077214e8a2c78807c1ab9/docs/http/http-metrics.md). +[metrics semantic conventions](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md). -Have you opt-ed into the new Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`? +A different metric is emitted depending if a user opts-in to the new Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`. -* If yes, the instrumentation supports the following metric. +* By default, the instrumentation emits the following metric. + + | Name | Instrument Type | Unit | Description | + |-------|-----------------|------|-------------| + | `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. | + +* If user sets the environment variable to `http`, the instrumentation emits the following metric. | Name | Instrument Type | Unit | Description | |-------|-----------------|------|-------------| | `http.server.request.duration` | Histogram | `s` | Measures the duration of inbound HTTP requests. | This metric is emitted in `seconds` as per the semantic convention. While - the convention [recommends using custom histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration) + the convention [recommends using custom histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md) , this feature is not yet available via .NET Metrics API. A [workaround](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) has been included in OTel SDK starting version `1.6.0` which applies recommended buckets by default for this metric. -* 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. | +* If user sets the environment variable to `http/dup`, the instrumentation emits both `http.server.duration` and `http.server.request.duration`. ## Advanced configuration From 0cfa424d39acdce7d84e166b58398487984f8e58 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Wed, 13 Sep 2023 16:17:01 -0700 Subject: [PATCH 36/40] markdownlint --- src/OpenTelemetry.Instrumentation.AspNetCore/README.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index 856417bb242..116abe813d4 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -93,7 +93,8 @@ public void ConfigureServices(IServiceCollection services) The instrumentation is implemented based on [metrics semantic conventions](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md). -A different metric is emitted depending if a user opts-in to the new Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`. +A different metric is emitted depending if a user opts-in to the new Http +Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`. * By default, the instrumentation emits the following metric. @@ -101,7 +102,8 @@ A different metric is emitted depending if a user opts-in to the new Http Semant |-------|-----------------|------|-------------| | `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. | -* If user sets the environment variable to `http`, the instrumentation emits the following metric. +* If user sets the environment variable to `http`, the instrumentation emits + the following metric. | Name | Instrument Type | Unit | Description | |-------|-----------------|------|-------------| @@ -114,7 +116,8 @@ A different metric is emitted depending if a user opts-in to the new Http Semant has been included in OTel SDK starting version `1.6.0` which applies recommended buckets by default for this metric. -* If user sets the environment variable to `http/dup`, the instrumentation emits both `http.server.duration` and `http.server.request.duration`. +* If user sets the environment variable to `http/dup`, the instrumentation + emits both `http.server.duration` and `http.server.request.duration`. ## Advanced configuration From 80df848a35bed281fdec87e2beb68f91dbb04393 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 14 Sep 2023 13:49:06 -0700 Subject: [PATCH 37/40] rewrite changelog --- .../CHANGELOG.md | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 8aa187f1fa3..905919c7362 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,16 +2,15 @@ ## Unreleased -* Introduced a new metric, `http.server.request.duration`, for users who opt-in - to the new semantic convention by configuring the `OTEL_SEMCONV_STABILITY_OPT_IN` - environment variable. This metric measures time in seconds and uses unique - histogram buckets as defined by the semantic conventions for Http - [metrics](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md) - While the convention recommends using custom histogram buckets, this feature - is not yet available via .NET Metrics API. A - [workaround](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) - has been included in OTel SDK starting version `1.6.0` which applies - recommended buckets by default for this metric. +* Introduced a new metric, `http.server.request.duration` measured in seconds. + Starting in version 1.6.0, the OTel SDK + [applies custom histogram buckets](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) + for this metric to comply with the + [Semantic Convention for Http Metrics](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md). + This new metric is only available for users who opt-in to the new new + semantic convention by configuring the `OTEL_SEMCONV_STABILITY_OPT_IN` + environment variable to either `http` (to emit only the new metric) or + `http/dup` (to emit both the new and old metrics). ([#4802](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4802)) * New metric: `http.server.request.duration` * Unit: `seconds` @@ -22,6 +21,15 @@ * Histogram Buckets: `0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000` + Note: the older `http.server.duration` metric will eventually be removed + after the HTTP semantic conventions are marked stable. + At which time this instrumentation can receive a stable release, and the old + HTTP and network semantic conventions will no longer be supported. Refer to + the specification for more information regarding the new HTTP and network + semantic conventions for both spans and metrics. + + + ## 1.5.1-beta.1 Released 2023-Jul-20 From 744a980b8796724334e1a07f2d08c81aad7dd050 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 14 Sep 2023 14:18:18 -0700 Subject: [PATCH 38/40] edit changelog & readme --- .../CHANGELOG.md | 23 ++++++++++--------- .../README.md | 17 ++++++-------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 905919c7362..b5c619cb27d 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -3,32 +3,33 @@ ## Unreleased * Introduced a new metric, `http.server.request.duration` measured in seconds. - Starting in version 1.6.0, the OTel SDK + The OTel SDK [applies custom histogram buckets](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) for this metric to comply with the [Semantic Convention for Http Metrics](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md). - This new metric is only available for users who opt-in to the new new + This new metric is only available for users who opt-in to the new semantic convention by configuring the `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable to either `http` (to emit only the new metric) or - `http/dup` (to emit both the new and old metrics). + `http/dup` (to emit both the new and old metrics). ([#4802](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4802)) * New metric: `http.server.request.duration` - * Unit: `seconds` + * Unit: `s` (seconds) * Histogram Buckets: `0, 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` - * Replaces old metric: `http.server.duration` - * Unit: `miliseconds` + * 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: the older `http.server.duration` metric will eventually be removed after the HTTP semantic conventions are marked stable. At which time this instrumentation can receive a stable release, and the old - HTTP and network semantic conventions will no longer be supported. Refer to - the specification for more information regarding the new HTTP and network - semantic conventions for both spans and metrics. - - + HTTP semantic conventions will no longer be supported. Refer to + the specification for more information regarding the new HTTP + semantic conventions for both + [spans](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-spans.md) + and + [metrics](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md). ## 1.5.1-beta.1 diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index 116abe813d4..b3322d7dd04 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -90,31 +90,28 @@ public void ConfigureServices(IServiceCollection services) #### List of metrics produced -The instrumentation is implemented based on -[metrics semantic conventions](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md). - A different metric is emitted depending if a user opts-in to the new Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`. * By default, the instrumentation emits the following metric. - | Name | Instrument Type | Unit | Description | - |-------|-----------------|------|-------------| - | `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. | + | Name | Instrument Type | Unit | Description | Attributes | + |-------|-----------------|------|-------------|------------| + | `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. | http.flavor, http.scheme, http.method, http.status_code, net.host.name, net.host.port, http.route | * If user sets the environment variable to `http`, the instrumentation emits the following metric. - | Name | Instrument Type | Unit | Description | - |-------|-----------------|------|-------------| - | `http.server.request.duration` | Histogram | `s` | Measures the duration of inbound HTTP requests. | + | Name | Instrument Type | Unit | Description | Attributes | + |-------|-----------------|------|-------------|------------| + | `http.server.request.duration` | Histogram | `s` | Measures the duration of inbound HTTP requests. | network.protocol.version, url.scheme, http.request.method, http.response.status_code, http.route | This metric is emitted in `seconds` as per the semantic convention. While the convention [recommends using custom histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md) , this feature is not yet available via .NET Metrics API. A [workaround](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) has been included in OTel SDK starting version `1.6.0` which applies - recommended buckets by default for this metric. + recommended buckets by default for `http.server.request.duration`. * If user sets the environment variable to `http/dup`, the instrumentation emits both `http.server.duration` and `http.server.request.duration`. From 1d694bc4b0d7e448f2fb82c966134bebc3c9d301 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 14 Sep 2023 14:38:36 -0700 Subject: [PATCH 39/40] update --- .../CHANGELOG.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index b5c619cb27d..e1439a9c231 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -21,12 +21,12 @@ * Histogram Buckets: `0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000` - Note: the older `http.server.duration` metric will eventually be removed - after the HTTP semantic conventions are marked stable. - At which time this instrumentation can receive a stable release, and the old - HTTP semantic conventions will no longer be supported. Refer to - the specification for more information regarding the new HTTP - semantic conventions for both + Note: the older `http.server.duration` metric and + `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable will eventually be + removed after the HTTP semantic conventions are marked stable. + At which time this instrumentation can publish a stable release. Refer to + the specification for more information regarding the new HTTP semantic + conventions for both [spans](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-spans.md) and [metrics](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md). From 0015a20004a047e906ea12346ee1acddff9e8264 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 14 Sep 2023 15:37:52 -0700 Subject: [PATCH 40/40] feedback --- src/OpenTelemetry.Instrumentation.AspNetCore/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md index b3322d7dd04..257dc308278 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md @@ -90,8 +90,8 @@ public void ConfigureServices(IServiceCollection services) #### List of metrics produced -A different metric is emitted depending if a user opts-in to the new Http -Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`. +A different metric is emitted depending on whether a user opts-in to the new +Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`. * By default, the instrumentation emits the following metric.