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() {