From d76aa77011ad7696ff6bd7ae6843089824058c3c Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Fri, 27 Oct 2023 12:41:57 -0700 Subject: [PATCH 1/5] update test packages and global.json to net8.0 rc.2 (#5000) --- Directory.Packages.props | 4 ++-- global.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index 2496514b08e..b2c6b502676 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -102,7 +102,7 @@ - - + + diff --git a/global.json b/global.json index 27af8d6b9fb..d60c4af4241 100644 --- a/global.json +++ b/global.json @@ -1,6 +1,6 @@ { "sdk": { "rollForward": "latestFeature", - "version": "8.0.100-rc.1.23463.5" + "version": "8.0.0-rc.2.23480.2" } } From 86a6ba0b7f7ed1f5e84e5a6610e640989cd3ae9f Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Fri, 27 Oct 2023 13:39:44 -0700 Subject: [PATCH 2/5] fix global.json (#5002) --- global.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/global.json b/global.json index d60c4af4241..16c870f7f47 100644 --- a/global.json +++ b/global.json @@ -1,6 +1,6 @@ { "sdk": { "rollForward": "latestFeature", - "version": "8.0.0-rc.2.23480.2" + "version": "8.0.100-rc.2.23502.2" } } From b45b8a9a1272ea91924cc644fd6765ed44778ec6 Mon Sep 17 00:00:00 2001 From: ImoutoChan Date: Tue, 31 Oct 2023 06:19:14 +0300 Subject: [PATCH 3/5] Check isSampled parameter in UpdateWithExemplar method (#5004) --- src/OpenTelemetry/CHANGELOG.md | 4 +++ src/OpenTelemetry/Metrics/MetricPoint.cs | 44 +++++++++++++++--------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 30856d74dfc..e5d3afac461 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -11,6 +11,10 @@ `autoGenerateServiceInstanceId` is `true`. ([#4988](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4988)) +* Fixed a bug where isSampled parameter wasn't properly checked in certain cases + within the `UpdateWithExemplar` method of `MetricPoint`. + ([#4851](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5004)) + ## 1.7.0-alpha.1 Released 2023-Oct-16 diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index 48f726f4754..831f027b3eb 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -499,11 +499,14 @@ internal void UpdateWithExemplar(long number, ReadOnlySpan Date: Tue, 31 Oct 2023 11:50:58 -0700 Subject: [PATCH 4/5] [ASP.NET Core] Set `http.request.method` as per spec (#5001) --- OpenTelemetry.sln | 1 + .../CHANGELOG.md | 16 ++++ .../Implementation/HttpInListener.cs | 11 ++- .../Implementation/HttpInMetricsListener.cs | 5 +- .../MeterProviderBuilderExtensions.cs | 3 +- ...elemetry.Instrumentation.AspNetCore.csproj | 1 + .../TracerProviderBuilderExtensions.cs | 3 +- src/Shared/RequestMethodHelper.cs | 77 +++++++++++++++++++ src/Shared/SemanticConventions.cs | 1 + .../BasicTests.cs | 75 ++++++++++++++++++ .../MetricTests.cs | 76 ++++++++++++++++++ 11 files changed, 265 insertions(+), 4 deletions(-) create mode 100644 src/Shared/RequestMethodHelper.cs diff --git a/OpenTelemetry.sln b/OpenTelemetry.sln index 922ba6b2d20..e4908525cef 100644 --- a/OpenTelemetry.sln +++ b/OpenTelemetry.sln @@ -269,6 +269,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shared", "Shared", "{A49299 src\Shared\PeriodicExportingMetricReaderHelper.cs = src\Shared\PeriodicExportingMetricReaderHelper.cs src\Shared\PooledList.cs = src\Shared\PooledList.cs src\Shared\ResourceSemanticConventions.cs = src\Shared\ResourceSemanticConventions.cs + src\Shared\RequestMethodHelper.cs = src\Shared\RequestMethodHelper.cs src\Shared\SemanticConventions.cs = src\Shared\SemanticConventions.cs src\Shared\SpanAttributeConstants.cs = src\Shared\SpanAttributeConstants.cs src\Shared\SpanHelper.cs = src\Shared\SpanHelper.cs diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index eb65dfb92c8..72f9147b46e 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,6 +2,22 @@ ## Unreleased +* Updated `http.request.method` to match specification guidelines. + * For activity, if the method does not belong to one of the [known + values](https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#:~:text=http.request.method%20has%20the%20following%20list%20of%20well%2Dknown%20values) + then the request method will be set on an additional tag + `http.request.method.original` and `http.request.method` will be set to + `_OTHER`. + * For metrics, if the original method does not belong to one of the [known + values](https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#:~:text=http.request.method%20has%20the%20following%20list%20of%20well%2Dknown%20values) + then `http.request.method` on `http.server.request.duration` metric will be + set to `_OTHER` + + `http.request.method` is set on `http.server.request.duration` metric or + activity when `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable is set to + `http` or `http/dup`. + ([#5001](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5001)) + ## 1.6.0-beta.2 Released 2023-Oct-26 diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 3b25d781ee2..b074fb5c254 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -252,7 +252,16 @@ public void OnStartActivity(Activity activity, object payload) activity.SetTag(SemanticConventions.AttributeUrlQuery, request.QueryString.Value); } - activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, request.Method); + if (RequestMethodHelper.TryResolveHttpMethod(request.Method, out var httpMethod)) + { + activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, httpMethod); + } + else + { + activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, httpMethod); + activity.SetTag(SemanticConventions.AttributeHttpRequestMethodOriginal, request.Method); + } + activity.SetTag(SemanticConventions.AttributeUrlScheme, request.Scheme); activity.SetTag(SemanticConventions.AttributeUrlPath, path); activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(request.Protocol)); diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 07221945d6c..987fac68e57 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -18,6 +18,8 @@ using System.Diagnostics; using System.Diagnostics.Metrics; using Microsoft.AspNetCore.Http; +using OpenTelemetry.Internal; + #if NET6_0_OR_GREATER using Microsoft.AspNetCore.Routing; #endif @@ -150,8 +152,9 @@ public void OnEventWritten_New(string name, object payload) tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolName, NetworkProtocolName)); 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))); + RequestMethodHelper.TryResolveHttpMethod(context.Request.Method, out var httpMethod); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, httpMethod)); #if NET6_0_OR_GREATER var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs index 71f00d1b5bf..6118642b2ed 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs @@ -42,8 +42,9 @@ public static MeterProviderBuilder AddAspNetCoreInstrumentation( #if NET8_0_OR_GREATER return builder.ConfigureMeters(); #else - // Note: Warm-up the status code mapping. + // Note: Warm-up the status code and method mapping. _ = TelemetryHelper.BoxedStatusCodes; + _ = RequestMethodHelper.KnownMethods; builder.ConfigureServices(services => { diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj b/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj index 840652e39c4..19c3a53f73b 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj @@ -14,6 +14,7 @@ + diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs index dae87bf4991..b4749b699f7 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs @@ -63,8 +63,9 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation( { Guard.ThrowIfNull(builder); - // Note: Warm-up the status code mapping. + // Note: Warm-up the status code and method mapping. _ = TelemetryHelper.BoxedStatusCodes; + _ = RequestMethodHelper.KnownMethods; name ??= Options.DefaultName; diff --git a/src/Shared/RequestMethodHelper.cs b/src/Shared/RequestMethodHelper.cs new file mode 100644 index 00000000000..b4404bde770 --- /dev/null +++ b/src/Shared/RequestMethodHelper.cs @@ -0,0 +1,77 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +#if NET8_0_OR_GREATER +using System.Collections.Frozen; +#endif + +namespace OpenTelemetry.Internal; + +internal static class RequestMethodHelper +{ +#if NET8_0_OR_GREATER + internal static readonly FrozenDictionary KnownMethods; +#else + internal static readonly Dictionary KnownMethods; +#endif + + static RequestMethodHelper() + { +#if NET8_0_OR_GREATER + KnownMethods = FrozenDictionary.ToFrozenDictionary( + new[] + { + KeyValuePair.Create("GET", "GET"), + KeyValuePair.Create("PUT", "PUT"), + KeyValuePair.Create("POST", "POST"), + KeyValuePair.Create("DELETE", "DELETE"), + KeyValuePair.Create("HEAD", "HEAD"), + KeyValuePair.Create("OPTIONS", "OPTIONS"), + KeyValuePair.Create("TRACE", "TRACE"), + KeyValuePair.Create("PATCH", "PATCH"), + KeyValuePair.Create("CONNECT", "CONNECT"), + }, + StringComparer.OrdinalIgnoreCase); +#else + KnownMethods = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + { "GET", "GET" }, + { "PUT", "PUT" }, + { "POST", "POST" }, + { "DELETE", "DELETE" }, + { "HEAD", "HEAD" }, + { "OPTIONS", "OPTIONS" }, + { "TRACE", "TRACE" }, + { "PATCH", "PATCH" }, + { "CONNECT", "CONNECT" }, + }; +#endif + } + + public static bool TryResolveHttpMethod(string method, out string resolvedMethod) + { + if (KnownMethods.TryGetValue(method, out resolvedMethod)) + { + // KnownMethods ignores case. Use the value returned by the dictionary to have a consistent case. + return true; + } + + // Set to default "_OTHER" as per spec. + // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes + resolvedMethod = "_OTHER"; + return false; + } +} diff --git a/src/Shared/SemanticConventions.cs b/src/Shared/SemanticConventions.cs index d48f7d6bca1..26c016b3e1f 100644 --- a/src/Shared/SemanticConventions.cs +++ b/src/Shared/SemanticConventions.cs @@ -129,4 +129,5 @@ internal static class SemanticConventions public const string AttributeUrlScheme = "url.scheme"; // replaces: "http.scheme" (AttributeHttpScheme) public const string AttributeUrlQuery = "url.query"; public const string AttributeUserAgentOriginal = "user_agent.original"; // replaces: "http.user_agent" (AttributeHttpUserAgent) + public const string AttributeHttpRequestMethodOriginal = "http.request.method_original"; } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index 87a70d3bfbd..93ef8753772 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs @@ -21,6 +21,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 Moq; @@ -32,6 +33,8 @@ using TestApp.AspNetCore.Filters; using Xunit; +using static OpenTelemetry.Internal.HttpSemanticConventionHelper; + namespace OpenTelemetry.Instrumentation.AspNetCore.Tests; // See https://github.com/aspnet/Docs/tree/master/aspnetcore/test/integration-tests/samples/2.x/IntegrationTestsSample @@ -647,6 +650,78 @@ void ConfigureTestServices(IServiceCollection services) Assert.Equal("api/Values/{id}", aspnetcoreframeworkactivity.DisplayName); } + [Theory] + [InlineData("CONNECT", "CONNECT")] + [InlineData("DELETE", "DELETE")] + [InlineData("GET", "GET")] + [InlineData("PUT", "PUT")] + [InlineData("HEAD", "HEAD")] + [InlineData("OPTIONS", "OPTIONS")] + [InlineData("PATCH", "PATCH")] + [InlineData("Get", "GET")] + [InlineData("POST", "POST")] + [InlineData("TRACE", "TRACE")] + [InlineData("CUSTOM", "_OTHER")] + public async Task HttpRequestMethodIsSetAsPerSpec(string originalMethod, string expectedMethod) + { + var exportedItems = new List(); + + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { [SemanticConventionOptInKeyName] = "http" }) + .Build(); + + void ConfigureTestServices(IServiceCollection services) + { + this.tracerProvider = Sdk.CreateTracerProviderBuilder() + .ConfigureServices(services => services.AddSingleton(configuration)) + .AddAspNetCoreInstrumentation() + .AddInMemoryExporter(exportedItems) + .Build(); + } + + // Arrange + using var client = this.factory + .WithWebHostBuilder(builder => + { + builder.ConfigureTestServices(ConfigureTestServices); + builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); + }) + .CreateClient(); + + var message = new HttpRequestMessage(); + + message.Method = new HttpMethod(originalMethod); + + try + { + using var response = await client.SendAsync(message).ConfigureAwait(false); + response.EnsureSuccessStatusCode(); + } + catch + { + // ignore error. + } + + WaitForActivityExport(exportedItems, 1); + + Assert.Single(exportedItems); + + var activity = exportedItems[0]; + + Assert.Contains(activity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRequestMethod); + + if (originalMethod.Equals(expectedMethod, StringComparison.OrdinalIgnoreCase)) + { + Assert.DoesNotContain(activity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRequestMethodOriginal); + } + else + { + Assert.Equal(originalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal) as string); + } + + Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod) as string); + } + [Fact] public async Task ActivitiesStartedInMiddlewareBySettingHostActivityToNullShouldNotBeUpdated() { diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index ff11b5e1225..d6f72df7c62 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -237,6 +237,82 @@ public async Task RequestMetricIsCaptured_New() expectedTagsCount: 6); } + [Theory] + [InlineData("CONNECT", "CONNECT")] + [InlineData("DELETE", "DELETE")] + [InlineData("GET", "GET")] + [InlineData("PUT", "PUT")] + [InlineData("HEAD", "HEAD")] + [InlineData("OPTIONS", "OPTIONS")] + [InlineData("PATCH", "PATCH")] + [InlineData("Get", "GET")] + [InlineData("POST", "POST")] + [InlineData("TRACE", "TRACE")] + [InlineData("CUSTOM", "_OTHER")] + public async Task HttpRequestMethodIsCapturedAsPerSpec(string originalMethod, string expectedMethod) + { + 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 message = new HttpRequestMessage(); + message.Method = new HttpMethod(originalMethod); + + try + { + using var response = await client.SendAsync(message).ConfigureAwait(false); + } + catch + { + // ignore error. + } + + // 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(); + + 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.Single(metricPoints); + + var mp = metricPoints[0]; + + // Inspect Metric Attributes + var attributes = new Dictionary(); + foreach (var tag in mp.Tags) + { + attributes[tag.Key] = tag.Value; + } + + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpRequestMethod && kvp.Value.ToString() == expectedMethod); + + Assert.DoesNotContain(attributes, t => t.Key == SemanticConventions.AttributeHttpRequestMethodOriginal); + } + #if !NET8_0_OR_GREATER [Fact] public async Task RequestMetricIsCaptured_Old() From d1d6d4b720c5c6a63042ac71c59fc6c96fd0b805 Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Tue, 31 Oct 2023 18:16:23 -0700 Subject: [PATCH 5/5] [HttpClient & HttpWebRequest] Set `http.request.method` as per spec (#5003) --- .../Implementation/HttpInListener.cs | 6 +- .../Implementation/HttpInMetricsListener.cs | 14 +- .../CHANGELOG.md | 16 +++ .../HttpHandlerDiagnosticListener.cs | 14 +- .../HttpHandlerMetricsDiagnosticListener.cs | 13 +- .../HttpWebRequestActivitySource.netfx.cs | 26 +++- .../MeterProviderBuilderExtensions.cs | 3 +- .../OpenTelemetry.Instrumentation.Http.csproj | 1 + .../TracerProviderBuilderExtensions.cs | 3 +- src/Shared/RequestMethodHelper.cs | 38 +---- .../HttpClientTests.Basic.cs | 133 ++++++++++++++++++ 11 files changed, 224 insertions(+), 43 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index b074fb5c254..83ba919385b 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -252,13 +252,15 @@ public void OnStartActivity(Activity activity, object payload) activity.SetTag(SemanticConventions.AttributeUrlQuery, request.QueryString.Value); } - if (RequestMethodHelper.TryResolveHttpMethod(request.Method, out var httpMethod)) + if (RequestMethodHelper.KnownMethods.TryGetValue(request.Method, out var httpMethod)) { activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, httpMethod); } else { - activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, httpMethod); + // Set to default "_OTHER" as per spec. + // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes + activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, "_OTHER"); activity.SetTag(SemanticConventions.AttributeHttpRequestMethodOriginal, request.Method); } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 987fac68e57..d78628f3b5d 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -14,7 +14,6 @@ // limitations under the License. // -#if !NET8_0_OR_GREATER using System.Diagnostics; using System.Diagnostics.Metrics; using Microsoft.AspNetCore.Http; @@ -153,8 +152,16 @@ public void OnEventWritten_New(string name, object payload) 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.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); - RequestMethodHelper.TryResolveHttpMethod(context.Request.Method, out var httpMethod); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, httpMethod)); + if (RequestMethodHelper.KnownMethods.TryGetValue(context.Request.Method, out var httpMethod)) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, httpMethod)); + } + else + { + // Set to default "_OTHER" as per spec. + // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, "_OTHER")); + } #if NET6_0_OR_GREATER var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; @@ -170,4 +177,3 @@ public void OnEventWritten_New(string name, object payload) this.httpServerRequestDuration.Record(Activity.Current.Duration.TotalSeconds, tags); } } -#endif diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 8229a7becd2..4b7bbb8cd3e 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -2,6 +2,22 @@ ## Unreleased +* Updated `http.request.method` to match specification guidelines. + * For activity, if the method does not belong to one of the [known + values](https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#:~:text=http.request.method%20has%20the%20following%20list%20of%20well%2Dknown%20values) + then the request method will be set on an additional tag + `http.request.method.original` and `http.request.method` will be set to + `_OTHER`. + * For metrics, if the original method does not belong to one of the [known + values](https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#:~:text=http.request.method%20has%20the%20following%20list%20of%20well%2Dknown%20values) + then `http.request.method` on `http.client.request.duration` metric will be + set to `_OTHER` + + `http.request.method` is set on `http.client.request.duration` metric or + activity when `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable is set to + `http` or `http/dup`. + ([#5003](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5003)) + ## 1.6.0-beta.2 Released 2023-Oct-26 diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index 40bf9a3fab6..b5d4c85fc5e 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -23,6 +23,7 @@ #endif using System.Reflection; using OpenTelemetry.Context.Propagation; +using OpenTelemetry.Internal; using OpenTelemetry.Trace; using static OpenTelemetry.Internal.HttpSemanticConventionHelper; @@ -185,7 +186,18 @@ public void OnStartActivity(Activity activity, object payload) // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md if (this.emitNewAttributes) { - activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, HttpTagHelper.GetNameForHttpMethod(request.Method)); + if (RequestMethodHelper.KnownMethods.TryGetValue(request.Method.Method, out var httpMethod)) + { + activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, httpMethod); + } + else + { + // Set to default "_OTHER" as per spec. + // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes + activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, "_OTHER"); + activity.SetTag(SemanticConventions.AttributeHttpRequestMethodOriginal, request.Method.Method); + } + activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); if (!request.RequestUri.IsDefaultPort) { diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs index 89266ce9139..eab4c6d4dc4 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs @@ -23,6 +23,7 @@ using System.Net.Http; #endif using System.Reflection; +using OpenTelemetry.Internal; using OpenTelemetry.Trace; using static OpenTelemetry.Internal.HttpSemanticConventionHelper; @@ -97,7 +98,17 @@ public override void OnEventWritten(string name, object payload) { TagList tags = default; - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, HttpTagHelper.GetNameForHttpMethod(request.Method))); + if (RequestMethodHelper.KnownMethods.TryGetValue(request.Method.Method, out var httpMethod)) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, httpMethod)); + } + else + { + // Set to default "_OTHER" as per spec. + // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, "_OTHER")); + } + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version))); tags.Add(new KeyValuePair(SemanticConventions.AttributeServerAddress, request.RequestUri.Host)); tags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, request.RequestUri.Scheme)); diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 2cb52a8bbd6..d2c23f6c6bf 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -23,6 +23,7 @@ using System.Reflection.Emit; using System.Runtime.CompilerServices; using OpenTelemetry.Context.Propagation; +using OpenTelemetry.Internal; using OpenTelemetry.Trace; using static OpenTelemetry.Internal.HttpSemanticConventionHelper; @@ -153,7 +154,18 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md if (tracingEmitNewAttributes) { - activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, request.Method); + if (RequestMethodHelper.KnownMethods.TryGetValue(request.Method, out var httpMethod)) + { + activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, httpMethod); + } + else + { + // Set to default "_OTHER" as per spec. + // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes + activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, "_OTHER"); + activity.SetTag(SemanticConventions.AttributeHttpRequestMethodOriginal, request.Method); + } + activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); if (!request.RequestUri.IsDefaultPort) { @@ -495,7 +507,17 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC { TagList tags = default; - tags.Add(SemanticConventions.AttributeHttpRequestMethod, request.Method); + if (RequestMethodHelper.KnownMethods.TryGetValue(request.Method, out var httpMethod)) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, httpMethod)); + } + else + { + // Set to default "_OTHER" as per spec. + // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, "_OTHER")); + } + tags.Add(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); tags.Add(SemanticConventions.AttributeUrlScheme, request.RequestUri.Scheme); tags.Add(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)); diff --git a/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs index 0890a662c8b..73b7ca44c0a 100644 --- a/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs @@ -45,8 +45,9 @@ public static MeterProviderBuilder AddHttpClientInstrumentation( .AddMeter("System.Net.Http") .AddMeter("System.Net.NameResolution"); #else - // Note: Warm-up the status code mapping. + // Note: Warm-up the status code and method mapping. _ = TelemetryHelper.BoxedStatusCodes; + _ = RequestMethodHelper.KnownMethods; builder.ConfigureServices(services => { diff --git a/src/OpenTelemetry.Instrumentation.Http/OpenTelemetry.Instrumentation.Http.csproj b/src/OpenTelemetry.Instrumentation.Http/OpenTelemetry.Instrumentation.Http.csproj index c252a0b5e26..1e18cd01855 100644 --- a/src/OpenTelemetry.Instrumentation.Http/OpenTelemetry.Instrumentation.Http.csproj +++ b/src/OpenTelemetry.Instrumentation.Http/OpenTelemetry.Instrumentation.Http.csproj @@ -16,6 +16,7 @@ + diff --git a/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs index 9429819b8a2..f8bb13a615c 100644 --- a/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs @@ -60,8 +60,9 @@ public static TracerProviderBuilder AddHttpClientInstrumentation( { Guard.ThrowIfNull(builder); - // Note: Warm-up the status code mapping. + // Note: Warm-up the status code and method mapping. _ = TelemetryHelper.BoxedStatusCodes; + _ = RequestMethodHelper.KnownMethods; name ??= Options.DefaultName; diff --git a/src/Shared/RequestMethodHelper.cs b/src/Shared/RequestMethodHelper.cs index b4404bde770..8ae14f89d0b 100644 --- a/src/Shared/RequestMethodHelper.cs +++ b/src/Shared/RequestMethodHelper.cs @@ -30,23 +30,7 @@ internal static class RequestMethodHelper static RequestMethodHelper() { -#if NET8_0_OR_GREATER - KnownMethods = FrozenDictionary.ToFrozenDictionary( - new[] - { - KeyValuePair.Create("GET", "GET"), - KeyValuePair.Create("PUT", "PUT"), - KeyValuePair.Create("POST", "POST"), - KeyValuePair.Create("DELETE", "DELETE"), - KeyValuePair.Create("HEAD", "HEAD"), - KeyValuePair.Create("OPTIONS", "OPTIONS"), - KeyValuePair.Create("TRACE", "TRACE"), - KeyValuePair.Create("PATCH", "PATCH"), - KeyValuePair.Create("CONNECT", "CONNECT"), - }, - StringComparer.OrdinalIgnoreCase); -#else - KnownMethods = new Dictionary(StringComparer.OrdinalIgnoreCase) + var knownMethodSet = new Dictionary(StringComparer.OrdinalIgnoreCase) { { "GET", "GET" }, { "PUT", "PUT" }, @@ -58,20 +42,12 @@ static RequestMethodHelper() { "PATCH", "PATCH" }, { "CONNECT", "CONNECT" }, }; -#endif - } - - public static bool TryResolveHttpMethod(string method, out string resolvedMethod) - { - if (KnownMethods.TryGetValue(method, out resolvedMethod)) - { - // KnownMethods ignores case. Use the value returned by the dictionary to have a consistent case. - return true; - } - // Set to default "_OTHER" as per spec. - // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes - resolvedMethod = "_OTHER"; - return false; + // KnownMethods ignores case. Use the value returned by the dictionary to have a consistent case. +#if NET8_0_OR_GREATER + KnownMethods = FrozenDictionary.ToFrozenDictionary(knownMethodSet, StringComparer.OrdinalIgnoreCase); +#else + KnownMethods = knownMethodSet; +#endif } } diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs index e039f25ac12..ea55780d955 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs @@ -15,6 +15,7 @@ // using System.Diagnostics; +using Microsoft.Extensions.Configuration; #if NETFRAMEWORK using System.Net; using System.Net.Http; @@ -23,11 +24,14 @@ using Moq; using OpenTelemetry.Context.Propagation; using OpenTelemetry.Instrumentation.Http.Implementation; +using OpenTelemetry.Metrics; using OpenTelemetry.Tests; using OpenTelemetry.Trace; using Xunit; using Xunit.Abstractions; +using static OpenTelemetry.Internal.HttpSemanticConventionHelper; + namespace OpenTelemetry.Instrumentation.Http.Tests; public partial class HttpClientTests : IDisposable @@ -368,6 +372,135 @@ public async Task ExportsSpansCreatedForRetries() Assert.NotEqual(spanid2, spanid3); } + [Theory] + [InlineData("CONNECT", "CONNECT")] + [InlineData("DELETE", "DELETE")] + [InlineData("GET", "GET")] + [InlineData("PUT", "PUT")] + [InlineData("HEAD", "HEAD")] + [InlineData("OPTIONS", "OPTIONS")] + [InlineData("PATCH", "PATCH")] + [InlineData("Get", "GET")] + [InlineData("POST", "POST")] + [InlineData("TRACE", "TRACE")] + [InlineData("CUSTOM", "_OTHER")] + public async Task HttpRequestMethodIsSetOnActivityAsPerSpec(string originalMethod, string expectedMethod) + { + var exportedItems = new List(); + using var request = new HttpRequestMessage + { + RequestUri = new Uri(this.url), + Method = new HttpMethod(originalMethod), + }; + + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { [SemanticConventionOptInKeyName] = "http" }) + .Build(); + + using var traceprovider = Sdk.CreateTracerProviderBuilder() + .ConfigureServices(services => services.AddSingleton(configuration)) + .AddHttpClientInstrumentation() + .AddInMemoryExporter(exportedItems) + .Build(); + + using var httpClient = new HttpClient(); + + try + { + await httpClient.SendAsync(request).ConfigureAwait(false); + } + catch + { + // ignore error. + } + + Assert.Single(exportedItems); + + var activity = exportedItems[0]; + + Assert.Contains(activity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRequestMethod); + + if (originalMethod.Equals(expectedMethod, StringComparison.OrdinalIgnoreCase)) + { + Assert.DoesNotContain(activity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRequestMethodOriginal); + } + else + { + Assert.Equal(originalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal) as string); + } + + Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod) as string); + } + + [Theory] + [InlineData("CONNECT", "CONNECT")] + [InlineData("DELETE", "DELETE")] + [InlineData("GET", "GET")] + [InlineData("PUT", "PUT")] + [InlineData("HEAD", "HEAD")] + [InlineData("OPTIONS", "OPTIONS")] + [InlineData("PATCH", "PATCH")] + [InlineData("Get", "GET")] + [InlineData("POST", "POST")] + [InlineData("TRACE", "TRACE")] + [InlineData("CUSTOM", "_OTHER")] + public async Task HttpRequestMethodIsSetonRequestDurationMetricAsPerSpec(string originalMethod, string expectedMethod) + { + var metricItems = new List(); + using var request = new HttpRequestMessage + { + RequestUri = new Uri(this.url), + Method = new HttpMethod(originalMethod), + }; + + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { [SemanticConventionOptInKeyName] = "http" }) + .Build(); + + using var meterprovider = Sdk.CreateMeterProviderBuilder() + .ConfigureServices(services => services.AddSingleton(configuration)) + .AddHttpClientInstrumentation() + .AddInMemoryExporter(metricItems) + .Build(); + + using var httpClient = new HttpClient(); + + try + { + await httpClient.SendAsync(request).ConfigureAwait(false); + } + catch + { + // ignore error. + } + + meterprovider.Dispose(); + + var metric = metricItems.FirstOrDefault(m => m.Name == "http.client.request.duration"); + + Assert.NotNull(metric); + + var metricPoints = new List(); + foreach (var p in metric.GetMetricPoints()) + { + metricPoints.Add(p); + } + + Assert.Single(metricPoints); + var mp = metricPoints[0]; + + // Inspect Metric Attributes + var attributes = new Dictionary(); + foreach (var tag in mp.Tags) + { + attributes[tag.Key] = tag.Value; + } + + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpRequestMethod && kvp.Value.ToString() == expectedMethod); + + Assert.DoesNotContain(attributes, t => t.Key == SemanticConventions.AttributeHttpRequestMethodOriginal); + } + [Fact] public async Task RedirectTest() {