From f6c4195057fddbf0f475c8db263ea3485ccf9894 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Date: Wed, 27 Nov 2024 12:25:53 -0800 Subject: [PATCH] [Instrumentation.Http] Skip tagging traces when running on .NET 9+ (#2314) Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com> --- .../CHANGELOG.md | 5 ++ .../HttpHandlerDiagnosticListener.cs | 51 +++++++++---------- .../README.md | 8 +++ .../HttpClientTests.Basic.cs | 20 ++++++++ .../HttpClientTests.cs | 18 +++++++ .../HttpWebRequestTests.cs | 10 ++++ 6 files changed, 84 insertions(+), 28 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index beb4f6db93..4dfe8ff572 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -8,6 +8,11 @@ * Updated OpenTelemetry core component version(s) to `1.10.0`. ([#2317](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2317)) +* Trace instrumentation no longer sets attributes when running on .NET 9 and + greater because `HttpClient` now includes native instrumentation which adds + attributes directly. + ([#2314](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2314)) + ## 1.9.0 Released 2024-Jun-17 diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index f7b98fac07..ff2e971456 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -20,7 +20,8 @@ internal sealed class HttpHandlerDiagnosticListener : ListenerHandler #endif internal static readonly AssemblyName AssemblyName = typeof(HttpHandlerDiagnosticListener).Assembly.GetName(); - internal static readonly bool IsNet7OrGreater = InitializeIsNet7OrGreater(); + internal static readonly bool IsNet7OrGreater = Environment.Version.Major >= 7; + internal static readonly bool IsNet9OrGreater = Environment.Version.Major >= 9; // https://github.com/dotnet/runtime/blob/7d034ddbbbe1f2f40c264b323b3ed3d6b3d45e9a/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L19 internal static readonly string ActivitySourceName = AssemblyName.Name + ".HttpClient"; @@ -35,6 +36,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; public HttpHandlerDiagnosticListener(HttpClientTraceInstrumentationOptions options) @@ -135,15 +137,17 @@ public void OnStartActivity(Activity activity, object? payload) ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Client); } - // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md - HttpTagHelper.RequestDataHelper.SetHttpMethodTag(activity, request.Method.Method); - - if (request.RequestUri != null) + if (!IsNet9OrGreater) { - activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); - activity.SetTag(SemanticConventions.AttributeServerPort, request.RequestUri.Port); + // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md + HttpTagHelper.RequestDataHelper.SetHttpMethodTag(activity, request.Method.Method); - activity.SetTag(SemanticConventions.AttributeUrlFull, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri, this.options.DisableUrlQueryRedaction)); + if (request.RequestUri != null) + { + activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); + activity.SetTag(SemanticConventions.AttributeServerPort, request.RequestUri.Port); + activity.SetTag(SemanticConventions.AttributeUrlFull, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri, this.options.DisableUrlQueryRedaction)); + } } try @@ -199,16 +203,19 @@ public void OnStopActivity(Activity activity, object? payload) if (TryFetchResponse(payload, out var response)) { - if (currentStatusCode == ActivityStatusCode.Unset) + if (!IsNet9OrGreater) { - activity.SetStatus(SpanHelper.ResolveActivityStatusForHttpStatusCode(activity.Kind, (int)response.StatusCode)); - } + if (currentStatusCode == ActivityStatusCode.Unset) + { + activity.SetStatus(SpanHelper.ResolveActivityStatusForHttpStatusCode(activity.Kind, (int)response.StatusCode)); + } - activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, RequestDataHelper.GetHttpProtocolVersion(response.Version)); - activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); - if (activity.Status == ActivityStatusCode.Error) - { - activity.SetTag(SemanticConventions.AttributeErrorType, TelemetryHelper.GetStatusCodeString(response.StatusCode)); + activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, RequestDataHelper.GetHttpProtocolVersion(response.Version)); + activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); + if (activity.Status == ActivityStatusCode.Error) + { + activity.SetTag(SemanticConventions.AttributeErrorType, TelemetryHelper.GetStatusCodeString(response.StatusCode)); + } } try @@ -323,16 +330,4 @@ static bool TryFetchException(object? payload, [NotNullWhen(true)] out Exception #endif return exc.GetType().FullName; } - - private static bool InitializeIsNet7OrGreater() - { - try - { - return typeof(HttpClient).Assembly.GetName().Version!.Major >= 7; - } - catch (Exception) - { - return false; - } - } } diff --git a/src/OpenTelemetry.Instrumentation.Http/README.md b/src/OpenTelemetry.Instrumentation.Http/README.md index cf729c8252..df39d39614 100644 --- a/src/OpenTelemetry.Instrumentation.Http/README.md +++ b/src/OpenTelemetry.Instrumentation.Http/README.md @@ -40,6 +40,14 @@ HTTP instrumentation must be enabled at application startup. #### Traces +Starting with .NET 9, trace instrumentation is natively implemented, and the +HttpClient library emits attributes defined in the +[OpenTelemetry Specification](https://github.com/open-telemetry/semantic-conventions/blob/v1.28.0/docs/http/http-spans.md). +When running on .NET 9+ this instrumentation library will not add/change/override +any attributes set by the native instrumentation but it is still required for +performing context propagation using the OpenTelemetry SDK and supports additional +features not available in runtime (enrichment, filtering, etc.). + The following example demonstrates adding `HttpClient` instrumentation with the extension method `.AddHttpClientInstrumentation()` on `TracerProviderBuilder` to a console application. This example also sets up the OpenTelemetry Console diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs index 0ee5a4b4c7..af7413b149 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs @@ -425,6 +425,18 @@ public async Task HttpRequestMethodIsSetOnActivityAsPerSpec(string originalMetho } Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod)); + +#if NET9_0_OR_GREATER + if (expectedOriginalMethod is not null and not "CUSTOM") + { + // HACK: THIS IS A HACK TO MAKE THE TEST PASS. + // TODO: THIS CAN BE REMOVED AFTER RUNTIME PATCHES NET9. + // Currently Runtime is not following the OTel Spec for Http Spans: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md#http-client + // Currently "http.request.method_original" is not being set as expected. + // Tracking issue: https://github.com/dotnet/runtime/issues/109847 + expectedOriginalMethod = null; + } +#endif Assert.Equal(expectedOriginalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal)); } @@ -727,6 +739,14 @@ public async Task ValidateUrlQueryRedaction(string urlQuery, string expectedUrlQ var activity = exportedItems[0]; var expectedUrl = $"{this.url}path{expectedUrlQuery}"; + +#if NET9_0_OR_GREATER + // HACK: THIS IS A HACK TO MAKE THE TEST PASS. + // TODO: NEED TO UPDATE THIS TEST TO USE .NET'S SETTING TO DISABLE REDACTION. + // Currently this doesn't work with our tests which run in parallel. + // For more information see: https://github.com/dotnet/docs/issues/42792 + expectedUrl = $"{this.url}path?*"; +#endif Assert.Equal(expectedUrl, activity.GetTagValue(SemanticConventions.AttributeUrlFull)); } diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index fd01551bf3..1ccf563a51 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -340,7 +340,25 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpRequestMethod && kvp.Value?.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpRequestMethod]); Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeServerAddress && kvp.Value?.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeServerAddress]); Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeServerPort && kvp.Value?.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeServerPort]); + +#if NET9_0_OR_GREATER + // HACK: THIS IS A HACK TO MAKE THE TEST PASS. + // TODO: THIS CAN BE REMOVED AFTER RUNTIME PATCHES NET9. + // Currently Runtime is not following the OTel Spec for Http Spans: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md#http-client + // Currently the URL Fragment Identifier (#fragment) isn't being recorded. + // Tracking issue: https://github.com/dotnet/runtime/issues/109847 + var expected = normalizedAttributesTestCase[SemanticConventions.AttributeUrlFull]; + if (expected.EndsWith("#fragment", StringComparison.Ordinal)) + { + // remove fragment from expected value + expected = expected.Substring(0, expected.Length - "#fragment".Length); + } + + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeUrlFull && kvp.Value?.ToString() == expected); +#else Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeUrlFull && kvp.Value?.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeUrlFull]); +#endif + if (tc.ResponseExpected) { Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeNetworkProtocolVersion && kvp.Value?.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetworkProtocolVersion]); diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.cs index 9e01f266c4..2445a6baf6 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.cs @@ -122,6 +122,16 @@ public void HttpOutCallsAreCollectedSuccessfully(HttpOutTestCase tc) Assert.Fail($"Tag {tag.Key} was not found in test data."); } +#if NET9_0_OR_GREATER + // TODO: NEED TO REVIEW THE SPEC + // NET9 does not record the URL Fragment Identifier. + if (value.EndsWith("#fragment", StringComparison.Ordinal)) + { + // remove fragment from expected value + value = value.Substring(0, value.Length - "#fragment".Length); + } +#endif + Assert.Equal(value, tagValue); }