From 81790f5c191cfddd9a3fc79dfd6976d23ad88430 Mon Sep 17 00:00:00 2001 From: Nils Gruson Date: Thu, 21 Dec 2023 18:46:20 +0100 Subject: [PATCH 01/14] Add support for OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS --- ...mentationMeterProviderBuilderExtensions.cs | 3 +- ...entationTracerProviderBuilderExtensions.cs | 1 - .../AspNetCoreTraceInstrumentationOptions.cs | 14 +++ .../Implementation/HttpInListener.cs | 18 +-- .../Implementation/HttpInMetricsListener.cs | 3 +- ...mentationMeterProviderBuilderExtensions.cs | 3 +- ...entationTracerProviderBuilderExtensions.cs | 3 +- .../HttpClientTraceInstrumentationOptions.cs | 10 ++ .../HttpHandlerDiagnosticListener.cs | 14 +-- .../HttpWebRequestActivitySource.netfx.cs | 4 +- src/Shared/RequestMethodHelper.cs | 92 +++++++++++---- .../BasicTests.cs | 107 ++++++++++++++++++ 12 files changed, 224 insertions(+), 48 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationMeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationMeterProviderBuilderExtensions.cs index 3da8c1de59d..35d9f85793f 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationMeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationMeterProviderBuilderExtensions.cs @@ -27,9 +27,8 @@ public static MeterProviderBuilder AddAspNetCoreInstrumentation( #if NET8_0_OR_GREATER return builder.ConfigureMeters(); #else - // Note: Warm-up the status code and method mapping. + // Note: Warm-up the status code. _ = TelemetryHelper.BoxedStatusCodes; - _ = RequestMethodHelper.KnownMethods; builder.AddMeter(HttpInMetricsListener.InstrumentationName); diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationTracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationTracerProviderBuilderExtensions.cs index d37d77ae3fa..0091729c70a 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationTracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationTracerProviderBuilderExtensions.cs @@ -52,7 +52,6 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation( // Note: Warm-up the status code and method mapping. _ = TelemetryHelper.BoxedStatusCodes; - _ = RequestMethodHelper.KnownMethods; name ??= Options.DefaultName; diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs index f66a5e04344..3603da27c55 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs @@ -1,6 +1,9 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#if NET8_0_OR_GREATER +using System.Collections.Frozen; +#endif using System.Diagnostics; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Configuration; @@ -29,6 +32,15 @@ internal AspNetCoreTraceInstrumentationOptions(IConfiguration configuration) { this.EnableGrpcAspNetCoreSupport = enableGrpcInstrumentation; } + + if (configuration.TryGetStringValue("OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS", out var knownHttpMethods)) + { + this.KnownHttpMethods = knownHttpMethods + .Split(',') + .Select(x => x.Trim()) + .Where(x => !string.IsNullOrEmpty(x)) + .ToList(); + } } /// @@ -91,4 +103,6 @@ internal AspNetCoreTraceInstrumentationOptions(IConfiguration configuration) /// https://github.com/open-telemetry/semantic-conventions/blob/main/docs/rpc/rpc-spans.md. /// internal bool EnableGrpcAspNetCoreSupport { get; set; } + + internal List KnownHttpMethods { get; set; } = new(); } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index a9aefacc005..12c1afeef78 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -58,6 +58,7 @@ internal class HttpInListener : ListenerHandler private readonly PropertyFetcher beforeActionTemplateFetcher = new("Template"); #endif private readonly AspNetCoreTraceInstrumentationOptions options; + private readonly RequestMethodHelper requestMethodHelper; public HttpInListener(AspNetCoreTraceInstrumentationOptions options) : base(DiagnosticSourceName) @@ -65,6 +66,7 @@ public HttpInListener(AspNetCoreTraceInstrumentationOptions options) Guard.ThrowIfNull(options); this.options = options; + this.requestMethodHelper = new RequestMethodHelper(this.options.KnownHttpMethods); } public override void OnEventWritten(string name, object payload) @@ -105,8 +107,7 @@ public void OnStartActivity(Activity activity, object payload) // By this time, samplers have already run and // activity.IsAllDataRequested populated accordingly. - HttpContext context = payload as HttpContext; - if (context == null) + if (payload is not HttpContext context) { AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStartActivity), activity.OperationName); return; @@ -176,7 +177,7 @@ public void OnStartActivity(Activity activity, object payload) #endif var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/"; - activity.DisplayName = GetDisplayName(request.Method); + activity.DisplayName = this.GetDisplayName(request.Method); // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md @@ -196,7 +197,7 @@ public void OnStartActivity(Activity activity, object payload) activity.SetTag(SemanticConventions.AttributeUrlQuery, request.QueryString.Value); } - RequestMethodHelper.SetHttpMethodTag(activity, request.Method); + this.requestMethodHelper.SetHttpMethodTag(activity, request.Method); activity.SetTag(SemanticConventions.AttributeUrlScheme, request.Scheme); activity.SetTag(SemanticConventions.AttributeUrlPath, path); @@ -226,8 +227,7 @@ public void OnStopActivity(Activity activity, object payload) { if (activity.IsAllDataRequested) { - HttpContext context = payload as HttpContext; - if (context == null) + if (payload is not HttpContext context) { AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStopActivity), activity.OperationName); return; @@ -239,7 +239,7 @@ public void OnStopActivity(Activity activity, object payload) var routePattern = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; if (!string.IsNullOrEmpty(routePattern)) { - activity.DisplayName = GetDisplayName(context.Request.Method, routePattern); + activity.DisplayName = this.GetDisplayName(context.Request.Method, routePattern); activity.SetTag(SemanticConventions.AttributeHttpRoute, routePattern); } #endif @@ -382,9 +382,9 @@ private static void AddGrpcAttributes(Activity activity, string grpcMethod, Http } } - private static string GetDisplayName(string httpMethod, string httpRoute = null) + private string GetDisplayName(string httpMethod, string httpRoute = null) { - var normalizedMethod = RequestMethodHelper.GetNormalizedHttpMethod(httpMethod); + var normalizedMethod = this.requestMethodHelper.GetNormalizedHttpMethod(httpMethod); return string.IsNullOrEmpty(httpRoute) ? normalizedMethod diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index c2afcd02a81..2577d0ba8a7 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -84,7 +84,8 @@ public static void OnStopEventWritten(string name, object payload) tags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, context.Request.Scheme)); tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); - var httpMethod = RequestMethodHelper.GetNormalizedHttpMethod(context.Request.Method); + var requestMethodHelper = new RequestMethodHelper(string.Empty); + var httpMethod = requestMethodHelper.GetNormalizedHttpMethod(context.Request.Method); tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, httpMethod)); #if NET6_0_OR_GREATER diff --git a/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationMeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationMeterProviderBuilderExtensions.cs index 24b9524696f..6629d61b654 100644 --- a/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationMeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationMeterProviderBuilderExtensions.cs @@ -32,9 +32,8 @@ public static MeterProviderBuilder AddHttpClientInstrumentation( .AddMeter("System.Net.Http") .AddMeter("System.Net.NameResolution"); #else - // Note: Warm-up the status code and method mapping. + // Note: Warm-up the status code. _ = TelemetryHelper.BoxedStatusCodes; - _ = RequestMethodHelper.KnownMethods; #if NETFRAMEWORK builder.AddMeter(HttpWebRequestActivitySource.MeterName); diff --git a/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationTracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationTracerProviderBuilderExtensions.cs index 6015c183f2c..6dd8acb772c 100644 --- a/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationTracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationTracerProviderBuilderExtensions.cs @@ -47,9 +47,8 @@ public static TracerProviderBuilder AddHttpClientInstrumentation( { Guard.ThrowIfNull(builder); - // Note: Warm-up the status code and method mapping. + // Note: Warm-up the status code. _ = TelemetryHelper.BoxedStatusCodes; - _ = RequestMethodHelper.KnownMethods; name ??= Options.DefaultName; diff --git a/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs index 95bb6dab7b0..c472790ac29 100644 --- a/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs @@ -1,6 +1,9 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#if NET8_0_OR_GREATER +using System.Collections.Frozen; +#endif using System.Diagnostics; using System.Net; #if NETFRAMEWORK @@ -8,6 +11,7 @@ #endif using System.Runtime.CompilerServices; using OpenTelemetry.Instrumentation.Http.Implementation; +using OpenTelemetry.Internal; namespace OpenTelemetry.Instrumentation.Http; @@ -125,6 +129,12 @@ public class HttpClientTraceInstrumentationOptions /// public bool RecordException { get; set; } +#if NET8_0_OR_GREATER + internal FrozenDictionary KnownHttpMethods { get; set; } +#else + internal Dictionary KnownHttpMethods { get; set; } = RequestMethodHelper.GetKnownMethods(null); +#endif + [MethodImpl(MethodImplOptions.AggressiveInlining)] internal bool EventFilterHttpRequestMessage(string activityName, object arg1) { diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index e0625b420db..278f4d48670 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -15,7 +15,7 @@ namespace OpenTelemetry.Instrumentation.Http.Implementation; -internal sealed class HttpHandlerDiagnosticListener : ListenerHandler +internal sealed class HttpHandlerDiagnosticListener(HttpClientTraceInstrumentationOptions options) : ListenerHandler("HttpHandlerDiagnosticListener") { internal static readonly AssemblyName AssemblyName = typeof(HttpHandlerDiagnosticListener).Assembly.GetName(); internal static readonly bool IsNet7OrGreater; @@ -34,7 +34,7 @@ internal sealed class HttpHandlerDiagnosticListener : ListenerHandler private static readonly PropertyFetcher StopResponseFetcher = new("Response"); private static readonly PropertyFetcher StopExceptionFetcher = new("Exception"); private static readonly PropertyFetcher StopRequestStatusFetcher = new("RequestTaskStatus"); - private readonly HttpClientTraceInstrumentationOptions options; + private readonly HttpClientTraceInstrumentationOptions options = options; static HttpHandlerDiagnosticListener() { @@ -48,12 +48,6 @@ static HttpHandlerDiagnosticListener() } } - public HttpHandlerDiagnosticListener(HttpClientTraceInstrumentationOptions options) - : base("HttpHandlerDiagnosticListener") - { - this.options = options; - } - public override void OnEventWritten(string name, object payload) { switch (name) @@ -135,7 +129,7 @@ public void OnStartActivity(Activity activity, object payload) return; } - RequestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method.Method); + RequestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method.Method, this.options.KnownHttpMethods); if (!IsNet7OrGreater) { @@ -144,7 +138,7 @@ public void OnStartActivity(Activity activity, object payload) } // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md - RequestMethodHelper.SetHttpMethodTag(activity, request.Method.Method); + RequestMethodHelper.SetHttpMethodTag(activity, request.Method.Method, this.options.KnownHttpMethods); activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); if (!request.RequestUri.IsDefaultPort) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index ea8e88abb5f..ed8f97b5237 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -95,12 +95,12 @@ internal static HttpClientTraceInstrumentationOptions TracingOptions [MethodImpl(MethodImplOptions.AggressiveInlining)] private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, Activity activity) { - RequestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method); + RequestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method, tracingOptions.KnownHttpMethods); if (activity.IsAllDataRequested) { // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md - RequestMethodHelper.SetHttpMethodTag(activity, request.Method); + RequestMethodHelper.SetHttpMethodTag(activity, request.Method, tracingOptions.KnownHttpMethods); activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); if (!request.RequestUri.IsDefaultPort) diff --git a/src/Shared/RequestMethodHelper.cs b/src/Shared/RequestMethodHelper.cs index 05a3f022511..556e3893b37 100644 --- a/src/Shared/RequestMethodHelper.cs +++ b/src/Shared/RequestMethodHelper.cs @@ -9,21 +9,13 @@ namespace OpenTelemetry.Internal; -internal static class RequestMethodHelper +internal class RequestMethodHelper { // The value "_OTHER" is used for non-standard HTTP methods. // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes public const string OtherHttpMethod = "_OTHER"; -#if NET8_0_OR_GREATER - internal static readonly FrozenDictionary KnownMethods; -#else - internal static readonly Dictionary KnownMethods; -#endif - - static RequestMethodHelper() - { - var knownMethodSet = new Dictionary(StringComparer.OrdinalIgnoreCase) + private static readonly Dictionary DefaultKnownMethods = new(StringComparer.OrdinalIgnoreCase) { { "GET", "GET" }, { "PUT", "PUT" }, @@ -36,24 +28,45 @@ static RequestMethodHelper() { "CONNECT", "CONNECT" }, }; - // 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); + private readonly FrozenDictionary knownMethods; #else - KnownMethods = knownMethodSet; + private Dictionary knownMethods; #endif + + public RequestMethodHelper(string configuredKnownMethods) + { + var splitArray = configuredKnownMethods.Split(',') + .Select(x => x.Trim()) + .Where(x => !string.IsNullOrEmpty(x)) + .ToList(); + + this.knownMethods = GetKnownMethods(splitArray); + } + + public RequestMethodHelper(List configuredKnownMethods) + { + this.knownMethods = GetKnownMethods(configuredKnownMethods); } - public static string GetNormalizedHttpMethod(string method) +#if NET8_0_OR_GREATER + public string GetNormalizedHttpMethod(string method) +#else + public string GetNormalizedHttpMethod(string method, Dictionary knownMethods = null) +#endif { - return KnownMethods.TryGetValue(method, out var normalizedMethod) + return this.knownMethods.TryGetValue(method, out var normalizedMethod) ? normalizedMethod : OtherHttpMethod; } - public static void SetHttpMethodTag(Activity activity, string method) +#if NET8_0_OR_GREATER + public void SetHttpMethodTag(Activity activity, string method) +#else + public void SetHttpMethodTag(Activity activity, string method) +#endif { - if (KnownMethods.TryGetValue(method, out var normalizedMethod)) + if (this.knownMethods.TryGetValue(method, out var normalizedMethod)) { activity?.SetTag(SemanticConventions.AttributeHttpRequestMethod, normalizedMethod); } @@ -64,9 +77,50 @@ public static void SetHttpMethodTag(Activity activity, string method) } } - public static void SetHttpClientActivityDisplayName(Activity activity, string method) +#if NET8_0_OR_GREATER + public void SetHttpClientActivityDisplayName(Activity activity, string method) +#else + public void SetHttpClientActivityDisplayName(Activity activity, string method, Dictionary knownMethods) +#endif { // https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md#name - activity.DisplayName = KnownMethods.TryGetValue(method, out var httpMethod) ? httpMethod : "HTTP"; + activity.DisplayName = this.knownMethods.TryGetValue(method, out var httpMethod) ? httpMethod : "HTTP"; + } + +#if NET8_0_OR_GREATER + private static FrozenDictionary GetKnownMethods(string configuredKnownMethods) +#else + private static Dictionary GetKnownMethods(string configuredKnownMethods) +#endif + { + var splitArray = configuredKnownMethods.Split(',') + .Select(x => x.Trim()) + .Where(x => !string.IsNullOrEmpty(x)) + .ToList(); + + return GetKnownMethods(splitArray); + } + +#if NET8_0_OR_GREATER + private static FrozenDictionary GetKnownMethods(List configuredKnownMethods) +#else + private static Dictionary GetKnownMethods(List configuredKnownMethods) +#endif + { + IEnumerable> knownMethods = DefaultKnownMethods; + + if (configuredKnownMethods != null) + { + if (configuredKnownMethods.Count > 0) + { + knownMethods = DefaultKnownMethods.Where(x => configuredKnownMethods.Contains(x.Key, StringComparer.InvariantCultureIgnoreCase)); + } + } + +#if NET8_0_OR_GREATER + return FrozenDictionary.ToFrozenDictionary(knownMethods, StringComparer.OrdinalIgnoreCase); +#else + return knownMethods.ToDictionary(x => x.Key, x => x.Value); +#endif } } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index 2d3f6d2c21a..649815e5ae1 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs @@ -8,6 +8,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.Hosting; using Microsoft.Extensions.Logging; @@ -1024,6 +1025,112 @@ public async Task DiagnosticSourceExceptionCallBackIsNotReceivedForExceptionsHan Assert.True(exceptionHandled); } + [Theory] + [InlineData("GET", null)] + public async Task KnownHttpMethodsAreBeingRespected_Defaults(string expectedMethod, string expectedOriginalMethod) + { + var exportedItems = new List(); + + using var client = this.factory + .WithWebHostBuilder(builder => + { + builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); + }) + .CreateClient(); + + this.tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddAspNetCoreInstrumentation() + .AddInMemoryExporter(exportedItems) + .Build(); + + using var request = new HttpRequestMessage(HttpMethod.Get, "/api/values"); + using var response = await client.SendAsync(request); + + Assert.Single(exportedItems); + var activity = exportedItems[0]; + + Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod) as string); + Assert.Equal(expectedOriginalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal) as string); + } + + [Theory] + [InlineData("GET,POST,PUT", "GET", null)] + [InlineData("POST,PUT", "_OTHER", "GET")] + [InlineData("fooBar", "_OTHER", "GET")] + [InlineData("", "GET", null)] + [InlineData(",", "GET", null)] + public async Task KnownHttpMethodsAreBeingRespected_EnvVar(string knownMethods, string expectedMethod, string expectedOriginalMethod) + { + var config = new KeyValuePair[] { new("OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS", knownMethods) }; + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(config) + .Build(); + + var exportedItems = new List(); + + using var client = this.factory + .WithWebHostBuilder(builder => + { + builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); + }) + .CreateClient(); + + this.tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddAspNetCoreInstrumentation() + .ConfigureServices(services => services.AddSingleton(configuration)) + .AddInMemoryExporter(exportedItems) + .Build(); + + using var request = new HttpRequestMessage(HttpMethod.Get, "/api/values"); + using var response = await client.SendAsync(request); + + Assert.Single(exportedItems); + var activity = exportedItems[0]; + + Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod) as string); + Assert.Equal(expectedOriginalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal) as string); + } + + [Theory] + [InlineData("GET,POST,PUT", "GET", null)] + [InlineData("POST,PUT", "_OTHER", "GET")] + [InlineData("fooBar", "_OTHER", "GET")] + public async Task KnownHttpMethodsAreBeingRespected_Programmatically(string knownMethods, string expectedMethod, string expectedOriginalMethod) + { + var exportedItems = new List(); + + using var client = this.factory + .WithWebHostBuilder(builder => + { + builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); + }) + .CreateClient(); + + this.tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddAspNetCoreInstrumentation(options => + { + var splitArray = knownMethods.Split(',') + .Select(x => x.Trim()) + .Where(x => !string.IsNullOrEmpty(x)); + + foreach (var item in splitArray) + { + options.KnownHttpMethods.Add(item); + } + }) + .AddInMemoryExporter(exportedItems) + .Build(); + + using var request = new HttpRequestMessage(HttpMethod.Get, "/api/values"); + using var response = await client.SendAsync(request); + + Assert.Single(exportedItems); + var activity = exportedItems[0]; + + Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod) as string); + Assert.Equal(expectedOriginalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal) as string); + } + public void Dispose() { this.tracerProvider?.Dispose(); From 22aad50ffb3dae7fd1cf3c021360971664606791 Mon Sep 17 00:00:00 2001 From: Nils Gruson Date: Thu, 21 Dec 2023 20:02:13 +0100 Subject: [PATCH 02/14] Added http unit tests --- ...entationTracerProviderBuilderExtensions.cs | 2 +- .../AspNetCoreTraceInstrumentationOptions.cs | 3 +- .../HttpClientTraceInstrumentationOptions.cs | 7 +-- .../HttpHandlerDiagnosticListener.cs | 5 +- .../HttpHandlerMetricsDiagnosticListener.cs | 7 +-- .../HttpWebRequestActivitySource.netfx.cs | 11 ++-- src/Shared/RequestMethodHelper.cs | 22 ++----- .../BasicTests.cs | 17 ++++-- .../HttpClientTests.Basic.cs | 59 +++++++++++++++---- 9 files changed, 80 insertions(+), 53 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationTracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationTracerProviderBuilderExtensions.cs index 0091729c70a..6c3d45fef0b 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationTracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationTracerProviderBuilderExtensions.cs @@ -50,7 +50,7 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation( { Guard.ThrowIfNull(builder); - // Note: Warm-up the status code and method mapping. + // Note: Warm-up the status code. _ = TelemetryHelper.BoxedStatusCodes; name ??= Options.DefaultName; diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs index 3603da27c55..571d06b8e7f 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs @@ -104,5 +104,6 @@ internal AspNetCoreTraceInstrumentationOptions(IConfiguration configuration) /// internal bool EnableGrpcAspNetCoreSupport { get; set; } - internal List KnownHttpMethods { get; set; } = new(); + internal List KnownHttpMethods { get; set; } = + []; } diff --git a/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs index c472790ac29..ca7e92fb100 100644 --- a/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs @@ -129,11 +129,8 @@ public class HttpClientTraceInstrumentationOptions /// public bool RecordException { get; set; } -#if NET8_0_OR_GREATER - internal FrozenDictionary KnownHttpMethods { get; set; } -#else - internal Dictionary KnownHttpMethods { get; set; } = RequestMethodHelper.GetKnownMethods(null); -#endif + internal List KnownHttpMethods { get; set; } = + []; [MethodImpl(MethodImplOptions.AggressiveInlining)] internal bool EventFilterHttpRequestMessage(string activityName, object arg1) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index 278f4d48670..f03a3a81b3a 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -35,6 +35,7 @@ internal sealed class HttpHandlerDiagnosticListener(HttpClientTraceInstrumentati private static readonly PropertyFetcher StopExceptionFetcher = new("Exception"); private static readonly PropertyFetcher StopRequestStatusFetcher = new("RequestTaskStatus"); private readonly HttpClientTraceInstrumentationOptions options = options; + private readonly RequestMethodHelper requestMethodHelper = new(options.KnownHttpMethods); static HttpHandlerDiagnosticListener() { @@ -129,7 +130,7 @@ public void OnStartActivity(Activity activity, object payload) return; } - RequestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method.Method, this.options.KnownHttpMethods); + this.requestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method.Method); if (!IsNet7OrGreater) { @@ -138,7 +139,7 @@ public void OnStartActivity(Activity activity, object payload) } // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md - RequestMethodHelper.SetHttpMethodTag(activity, request.Method.Method, this.options.KnownHttpMethods); + this.requestMethodHelper.SetHttpMethodTag(activity, 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 c6a710e2dd0..5cfa9cc8d2f 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs @@ -15,7 +15,7 @@ namespace OpenTelemetry.Instrumentation.Http.Implementation; -internal sealed class HttpHandlerMetricsDiagnosticListener : ListenerHandler +internal sealed class HttpHandlerMetricsDiagnosticListener(string name) : ListenerHandler(name) { internal const string OnStopEvent = "System.Net.Http.HttpRequestOut.Stop"; @@ -34,10 +34,7 @@ internal sealed class HttpHandlerMetricsDiagnosticListener : ListenerHandler private static readonly HttpRequestOptionsKey HttpRequestOptionsErrorKey = new(SemanticConventions.AttributeErrorType); #endif - public HttpHandlerMetricsDiagnosticListener(string name) - : base(name) - { - } + private static readonly RequestMethodHelper RequestMethodHelper = new(string.Empty); public static void OnStopEventWritten(Activity activity, object payload) { diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index ed8f97b5237..2f344e326c1 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -37,6 +37,7 @@ internal static class HttpWebRequestActivitySource private static readonly Meter WebRequestMeter = new(MeterName, Version); private static readonly Histogram HttpClientRequestDuration = WebRequestMeter.CreateHistogram("http.client.request.duration", "s", "Measures the duration of outbound HTTP requests."); + private static readonly RequestMethodHelper RequestMethodHelper; private static HttpClientTraceInstrumentationOptions tracingOptions; // Fields for reflection @@ -75,6 +76,7 @@ static HttpWebRequestActivitySource() PerformInjection(); TracingOptions = new HttpClientTraceInstrumentationOptions(); + RequestMethodHelper = new RequestMethodHelper(TracingOptions.KnownHttpMethods); } catch (Exception ex) { @@ -95,12 +97,12 @@ internal static HttpClientTraceInstrumentationOptions TracingOptions [MethodImpl(MethodImplOptions.AggressiveInlining)] private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, Activity activity) { - RequestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method, tracingOptions.KnownHttpMethods); + RequestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method); if (activity.IsAllDataRequested) { // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md - RequestMethodHelper.SetHttpMethodTag(activity, request.Method, tracingOptions.KnownHttpMethods); + RequestMethodHelper.SetHttpMethodTag(activity, request.Method); activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); if (!request.RequestUri.IsDefaultPort) @@ -359,12 +361,11 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC // Disposed HttpWebResponse throws when accessing properties, so let's make a copy of the data to ensure that doesn't happen. HttpWebResponse responseCopy = httpWebResponseCtor( - new object[] - { + [ uriAccessor(response), verbAccessor(response), coreResponseDataAccessor(response), mediaTypeAccessor(response), usesProxySemanticsAccessor(response), DecompressionMethods.None, isWebSocketResponseAccessor(response), connectionGroupNameAccessor(response), - }); + ]); if (activity != null) { diff --git a/src/Shared/RequestMethodHelper.cs b/src/Shared/RequestMethodHelper.cs index 556e3893b37..53b7abb43b4 100644 --- a/src/Shared/RequestMethodHelper.cs +++ b/src/Shared/RequestMethodHelper.cs @@ -52,7 +52,7 @@ public RequestMethodHelper(List configuredKnownMethods) #if NET8_0_OR_GREATER public string GetNormalizedHttpMethod(string method) #else - public string GetNormalizedHttpMethod(string method, Dictionary knownMethods = null) + public string GetNormalizedHttpMethod(string method) #endif { return this.knownMethods.TryGetValue(method, out var normalizedMethod) @@ -80,27 +80,13 @@ public void SetHttpMethodTag(Activity activity, string method) #if NET8_0_OR_GREATER public void SetHttpClientActivityDisplayName(Activity activity, string method) #else - public void SetHttpClientActivityDisplayName(Activity activity, string method, Dictionary knownMethods) + public void SetHttpClientActivityDisplayName(Activity activity, string method) #endif { // https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md#name activity.DisplayName = this.knownMethods.TryGetValue(method, out var httpMethod) ? httpMethod : "HTTP"; } -#if NET8_0_OR_GREATER - private static FrozenDictionary GetKnownMethods(string configuredKnownMethods) -#else - private static Dictionary GetKnownMethods(string configuredKnownMethods) -#endif - { - var splitArray = configuredKnownMethods.Split(',') - .Select(x => x.Trim()) - .Where(x => !string.IsNullOrEmpty(x)) - .ToList(); - - return GetKnownMethods(splitArray); - } - #if NET8_0_OR_GREATER private static FrozenDictionary GetKnownMethods(List configuredKnownMethods) #else @@ -113,14 +99,14 @@ private static Dictionary GetKnownMethods(List configure { if (configuredKnownMethods.Count > 0) { - knownMethods = DefaultKnownMethods.Where(x => configuredKnownMethods.Contains(x.Key, StringComparer.InvariantCultureIgnoreCase)); + knownMethods = DefaultKnownMethods.Where(x => configuredKnownMethods.Contains(x.Key, StringComparer.OrdinalIgnoreCase)); } } #if NET8_0_OR_GREATER return FrozenDictionary.ToFrozenDictionary(knownMethods, StringComparer.OrdinalIgnoreCase); #else - return knownMethods.ToDictionary(x => x.Key, x => x.Value); + return knownMethods.ToDictionary(x => x.Key, x => x.Value, StringComparer.OrdinalIgnoreCase); #endif } } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index 649815e5ae1..eb57fa7b24d 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs @@ -1068,6 +1068,12 @@ public async Task KnownHttpMethodsAreBeingRespected_EnvVar(string knownMethods, var exportedItems = new List(); + this.tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddAspNetCoreInstrumentation() + .ConfigureServices(services => services.AddSingleton(configuration)) + .AddInMemoryExporter(exportedItems) + .Build(); + using var client = this.factory .WithWebHostBuilder(builder => { @@ -1075,12 +1081,6 @@ public async Task KnownHttpMethodsAreBeingRespected_EnvVar(string knownMethods, }) .CreateClient(); - this.tracerProvider = Sdk.CreateTracerProviderBuilder() - .AddAspNetCoreInstrumentation() - .ConfigureServices(services => services.AddSingleton(configuration)) - .AddInMemoryExporter(exportedItems) - .Build(); - using var request = new HttpRequestMessage(HttpMethod.Get, "/api/values"); using var response = await client.SendAsync(request); @@ -1093,8 +1093,11 @@ public async Task KnownHttpMethodsAreBeingRespected_EnvVar(string knownMethods, [Theory] [InlineData("GET,POST,PUT", "GET", null)] + [InlineData("get,post,put", "GET", null)] [InlineData("POST,PUT", "_OTHER", "GET")] + [InlineData("post,put", "_OTHER", "GET")] [InlineData("fooBar", "_OTHER", "GET")] + [InlineData("foo,bar", "_OTHER", "GET")] public async Task KnownHttpMethodsAreBeingRespected_Programmatically(string knownMethods, string expectedMethod, string expectedOriginalMethod) { var exportedItems = new List(); @@ -1124,6 +1127,8 @@ public async Task KnownHttpMethodsAreBeingRespected_Programmatically(string know using var request = new HttpRequestMessage(HttpMethod.Get, "/api/values"); using var response = await client.SendAsync(request); + this.tracerProvider.Dispose(); + Assert.Single(exportedItems); var activity = exportedItems[0]; diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs index ba758f25103..cdd30e50c3d 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs @@ -6,6 +6,7 @@ using System.Net.Http; #endif using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; using OpenTelemetry.Context.Propagation; using OpenTelemetry.Instrumentation.Http.Implementation; using OpenTelemetry.Metrics; @@ -413,17 +414,17 @@ public async Task HttpRequestMethodIsSetOnActivityAsPerSpec(string originalMetho } [Theory] - [InlineData("CONNECT", "CONNECT")] - [InlineData("DELETE", "DELETE")] - [InlineData("GET", "GET")] - [InlineData("PUT", "PUT")] - [InlineData("HEAD", "HEAD")] - [InlineData("OPTIONS", "OPTIONS")] - [InlineData("PATCH", "PATCH")] + //[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")] + //[InlineData("POST", "POST")] + //[InlineData("TRACE", "TRACE")] + //[InlineData("CUSTOM", "_OTHER")] public async Task HttpRequestMethodIsSetonRequestDurationMetricAsPerSpec(string originalMethod, string expectedMethod) { var metricItems = new List(); @@ -732,6 +733,44 @@ public async Task CustomPropagatorCalled(bool sample, bool createParentActivity) #endif } + [Theory] + [InlineData("GET,POST,PUT", "GET", null)] + [InlineData("get,post,put", "GET", null)] + [InlineData("POST,PUT", "_OTHER", "GET")] + [InlineData("post,put", "_OTHER", "GET")] + [InlineData("fooBar", "_OTHER", "GET")] + [InlineData("foo,bar", "_OTHER", "GET")] + public async Task KnownHttpMethodsAreBeingRespected(string knownMethods, string expectedMethod, string expectedOriginalMethod) + { + var exportedItems = new List(); + + using (Sdk.CreateTracerProviderBuilder() + .AddHttpClientInstrumentation( + opt => + { + var splitArray = knownMethods.Split(',') + .Select(x => x.Trim()) + .Where(x => !string.IsNullOrEmpty(x)); + + foreach (var item in splitArray) + { + opt.KnownHttpMethods.Add(item); + } + }) + .AddInMemoryExporter(exportedItems) + .Build()) + { + using var c = new HttpClient(); + await c.GetAsync(this.url); + } + + Assert.Single(exportedItems); + var activity = exportedItems[0]; + + Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod) as string); + Assert.Equal(expectedOriginalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal) as string); + } + public void Dispose() { this.serverLifeTime?.Dispose(); From 2cf08b2448acfafb72b0b8608d2b71e387e0f8ea Mon Sep 17 00:00:00 2001 From: Nils Gruson Date: Thu, 21 Dec 2023 20:04:35 +0100 Subject: [PATCH 03/14] Removed unnecessary using statement --- .../HttpClientTraceInstrumentationOptions.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs index ca7e92fb100..29351c0ed3d 100644 --- a/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs @@ -11,7 +11,6 @@ #endif using System.Runtime.CompilerServices; using OpenTelemetry.Instrumentation.Http.Implementation; -using OpenTelemetry.Internal; namespace OpenTelemetry.Instrumentation.Http; From 5e4635e0b46e9435528867628af0569c3f86498e Mon Sep 17 00:00:00 2001 From: Nils Gruson Date: Thu, 21 Dec 2023 20:09:48 +0100 Subject: [PATCH 04/14] added inline data to tests --- .../HttpClientTests.Basic.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs index cdd30e50c3d..070733cdfb4 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs @@ -414,17 +414,17 @@ public async Task HttpRequestMethodIsSetOnActivityAsPerSpec(string originalMetho } [Theory] - //[InlineData("CONNECT", "CONNECT")] - //[InlineData("DELETE", "DELETE")] - //[InlineData("GET", "GET")] - //[InlineData("PUT", "PUT")] - //[InlineData("HEAD", "HEAD")] - //[InlineData("OPTIONS", "OPTIONS")] - //[InlineData("PATCH", "PATCH")] + [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")] + [InlineData("POST", "POST")] + [InlineData("TRACE", "TRACE")] + [InlineData("CUSTOM", "_OTHER")] public async Task HttpRequestMethodIsSetonRequestDurationMetricAsPerSpec(string originalMethod, string expectedMethod) { var metricItems = new List(); From 7bf90e5ff8e8c0b9c28a7a379c5d819eae360379 Mon Sep 17 00:00:00 2001 From: Nils Gruson Date: Thu, 21 Dec 2023 20:12:51 +0100 Subject: [PATCH 05/14] Removed unnecessary using statement --- .../HttpClientTraceInstrumentationOptions.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs index 29351c0ed3d..f919f3c588f 100644 --- a/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs @@ -1,9 +1,6 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -#if NET8_0_OR_GREATER -using System.Collections.Frozen; -#endif using System.Diagnostics; using System.Net; #if NETFRAMEWORK From 047b4b7a701b96e171b27b2db761cc49751ece34 Mon Sep 17 00:00:00 2001 From: Nils Gruson Date: Thu, 21 Dec 2023 20:16:09 +0100 Subject: [PATCH 06/14] Removed unnecessary using statement --- .../HttpClientTests.Basic.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs index 070733cdfb4..6f09e280e14 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs @@ -6,7 +6,6 @@ using System.Net.Http; #endif using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Options; using OpenTelemetry.Context.Propagation; using OpenTelemetry.Instrumentation.Http.Implementation; using OpenTelemetry.Metrics; From 3bc8c2d35c659c4880eed9e679b9140aafefb6b1 Mon Sep 17 00:00:00 2001 From: Nils Gruson Date: Thu, 21 Dec 2023 20:21:25 +0100 Subject: [PATCH 07/14] Removed unnecessary using statement --- .../AspNetCoreTraceInstrumentationOptions.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs index 571d06b8e7f..ac32b8f474d 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs @@ -1,9 +1,6 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -#if NET8_0_OR_GREATER -using System.Collections.Frozen; -#endif using System.Diagnostics; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Configuration; From 5e6d36c1e101b122dc80a345c38dd0205ae99a04 Mon Sep 17 00:00:00 2001 From: Nils Gruson Date: Thu, 21 Dec 2023 21:03:43 +0100 Subject: [PATCH 08/14] Fixed unit test http netfx --- .../HttpWebRequestActivitySource.netfx.cs | 10 ++++---- src/Shared/RequestMethodHelper.cs | 2 +- .../HttpClientTests.Basic.cs | 23 ++++++++++--------- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 2f344e326c1..622977b73ba 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -37,8 +37,8 @@ internal static class HttpWebRequestActivitySource private static readonly Meter WebRequestMeter = new(MeterName, Version); private static readonly Histogram HttpClientRequestDuration = WebRequestMeter.CreateHistogram("http.client.request.duration", "s", "Measures the duration of outbound HTTP requests."); - private static readonly RequestMethodHelper RequestMethodHelper; private static HttpClientTraceInstrumentationOptions tracingOptions; + private static RequestMethodHelper requestMethodHelper; // Fields for reflection private static FieldInfo connectionGroupListField; @@ -76,7 +76,6 @@ static HttpWebRequestActivitySource() PerformInjection(); TracingOptions = new HttpClientTraceInstrumentationOptions(); - RequestMethodHelper = new RequestMethodHelper(TracingOptions.KnownHttpMethods); } catch (Exception ex) { @@ -91,18 +90,19 @@ internal static HttpClientTraceInstrumentationOptions TracingOptions set { tracingOptions = value; + requestMethodHelper = new RequestMethodHelper(tracingOptions.KnownHttpMethods); } } [MethodImpl(MethodImplOptions.AggressiveInlining)] private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, Activity activity) { - RequestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method); + requestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method); if (activity.IsAllDataRequested) { // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md - RequestMethodHelper.SetHttpMethodTag(activity, request.Method); + requestMethodHelper.SetHttpMethodTag(activity, request.Method); activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); if (!request.RequestUri.IsDefaultPort) @@ -426,7 +426,7 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC TagList tags = default; - var httpMethod = RequestMethodHelper.GetNormalizedHttpMethod(request.Method); + var httpMethod = requestMethodHelper.GetNormalizedHttpMethod(request.Method); tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, httpMethod)); tags.Add(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); diff --git a/src/Shared/RequestMethodHelper.cs b/src/Shared/RequestMethodHelper.cs index 53b7abb43b4..9db6eee3925 100644 --- a/src/Shared/RequestMethodHelper.cs +++ b/src/Shared/RequestMethodHelper.cs @@ -31,7 +31,7 @@ internal class RequestMethodHelper #if NET8_0_OR_GREATER private readonly FrozenDictionary knownMethods; #else - private Dictionary knownMethods; + private readonly Dictionary knownMethods; #endif public RequestMethodHelper(string configuredKnownMethods) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs index 6f09e280e14..f99fff25425 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs @@ -733,17 +733,17 @@ public async Task CustomPropagatorCalled(bool sample, bool createParentActivity) } [Theory] - [InlineData("GET,POST,PUT", "GET", null)] - [InlineData("get,post,put", "GET", null)] - [InlineData("POST,PUT", "_OTHER", "GET")] - [InlineData("post,put", "_OTHER", "GET")] - [InlineData("fooBar", "_OTHER", "GET")] + //[InlineData("GET,POST,PUT", "GET", null)] + //[InlineData("get,post,put", "GET", null)] + //[InlineData("POST,PUT", "_OTHER", "GET")] + //[InlineData("post,put", "_OTHER", "GET")] + //[InlineData("fooBar", "_OTHER", "GET")] [InlineData("foo,bar", "_OTHER", "GET")] public async Task KnownHttpMethodsAreBeingRespected(string knownMethods, string expectedMethod, string expectedOriginalMethod) { var exportedItems = new List(); - using (Sdk.CreateTracerProviderBuilder() + var tracerProvider = Sdk.CreateTracerProviderBuilder() .AddHttpClientInstrumentation( opt => { @@ -757,11 +757,12 @@ public async Task KnownHttpMethodsAreBeingRespected(string knownMethods, string } }) .AddInMemoryExporter(exportedItems) - .Build()) - { - using var c = new HttpClient(); - await c.GetAsync(this.url); - } + .Build(); + + using var c = new HttpClient(); + await c.GetAsync(this.url); + + tracerProvider.Dispose(); Assert.Single(exportedItems); var activity = exportedItems[0]; From 688602fffb0811e3319647d265ba2c5505a3d228 Mon Sep 17 00:00:00 2001 From: Nils Gruson Date: Thu, 21 Dec 2023 21:10:22 +0100 Subject: [PATCH 09/14] Fixed netfx tests --- .../HttpClientTests.Basic.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs index f99fff25425..4c936f151ef 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs @@ -733,11 +733,11 @@ public async Task CustomPropagatorCalled(bool sample, bool createParentActivity) } [Theory] - //[InlineData("GET,POST,PUT", "GET", null)] - //[InlineData("get,post,put", "GET", null)] - //[InlineData("POST,PUT", "_OTHER", "GET")] - //[InlineData("post,put", "_OTHER", "GET")] - //[InlineData("fooBar", "_OTHER", "GET")] + [InlineData("GET,POST,PUT", "GET", null)] + [InlineData("get,post,put", "GET", null)] + [InlineData("POST,PUT", "_OTHER", "GET")] + [InlineData("post,put", "_OTHER", "GET")] + [InlineData("fooBar", "_OTHER", "GET")] [InlineData("foo,bar", "_OTHER", "GET")] public async Task KnownHttpMethodsAreBeingRespected(string knownMethods, string expectedMethod, string expectedOriginalMethod) { From f8e1d6d4b8625a91c3ce584533bd6682af252e6e Mon Sep 17 00:00:00 2001 From: Nils Gruson Date: Tue, 9 Jan 2024 13:15:36 +0100 Subject: [PATCH 10/14] Processed review comments --- Directory.Packages.props | 11 +--- .../AspNetCoreTraceInstrumentationOptions.cs | 8 +-- .../Implementation/HttpInMetricsListener.cs | 4 +- ...entationTracerProviderBuilderExtensions.cs | 2 + .../HttpClientTraceInstrumentationOptions.cs | 20 +++++++ .../OpenTelemetry.Instrumentation.Http.csproj | 3 + src/Shared/RequestMethodHelper.cs | 23 +++---- .../BasicTests.cs | 5 +- .../HttpClientTests.Basic.cs | 60 ++++++++++++++++++- 9 files changed, 106 insertions(+), 30 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index 8b4287b8901..967bab65572 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -3,7 +3,6 @@ true 1.7.0 - - + - @@ -42,7 +39,6 @@ - - - - - + - + @@ -39,6 +42,7 @@ + + + + +