From e904994fabe03079e6031dd33b1a3c5447ef8e79 Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Wed, 15 Nov 2023 15:21:00 -0800 Subject: [PATCH] [HttpClient & HttpWebRequest] Set network protocol version from response object (#5043) --- .../CHANGELOG.md | 6 ++++ .../HttpHandlerDiagnosticListener.cs | 2 +- .../HttpHandlerMetricsDiagnosticListener.cs | 2 +- .../HttpWebRequestActivitySource.netfx.cs | 19 ++++++++--- .../HttpClientTests.cs | 32 +++++++++---------- 5 files changed, 38 insertions(+), 23 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 1cf6558302c..f1e6a6bc999 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -44,6 +44,12 @@ * Fixed `network.protocol.version` attribute values to match the specification. ([#5006](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5006)) +* Set `network.protocol.version` value using the protocol version on the + received response. If the request fails without response, then + `network.protocol.version` attribute will not be set on Activity and + `http.client.request.duration` metric. + ([#5043](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5043)) + ## 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 924f260a90e..f5fb182211e 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -205,7 +205,6 @@ public void OnStartActivity(Activity activity, object payload) } activity.SetTag(SemanticConventions.AttributeUrlFull, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri)); - activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(request.Version)); if (request.Headers.TryGetValues("User-Agent", out var userAgentValues)) { @@ -283,6 +282,7 @@ public void OnStopActivity(Activity activity, object payload) if (this.emitNewAttributes) { + activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(response.Version)); activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); if (activity.Status == ActivityStatusCode.Error) { diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs index 9b20cbb533e..82bdc004ef3 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs @@ -130,7 +130,6 @@ public void OnStopEventWritten(Activity activity, object payload) tags.Add(new KeyValuePair(SemanticConventions.AttributeServerAddress, request.RequestUri.Host)); tags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, request.RequestUri.Scheme)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(request.Version))); if (!request.RequestUri.IsDefaultPort) { @@ -139,6 +138,7 @@ public void OnStopEventWritten(Activity activity, object payload) if (TryFetchResponse(payload, out HttpResponseMessage response)) { + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(response.Version))); tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode))); // Set error.type to status code for failed requests diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 3852201fde7..9a0fd30e78d 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -173,7 +173,6 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A } activity.SetTag(SemanticConventions.AttributeUrlFull, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri)); - activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(request.ProtocolVersion)); } try @@ -201,6 +200,7 @@ private static void AddResponseTags(HttpWebResponse response, Activity activity) if (tracingEmitNewAttributes) { + activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(response.ProtocolVersion)); activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); } @@ -243,11 +243,12 @@ private static string GetErrorType(Exception exception) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static void AddExceptionTags(Exception exception, Activity activity, out HttpStatusCode? statusCode) + private static void AddExceptionTags(Exception exception, Activity activity, out HttpStatusCode? statusCode, out Version protocolVersion) { Debug.Assert(activity != null, "Activity must not be null"); statusCode = null; + protocolVersion = null; if (!activity.IsAllDataRequested) { @@ -259,6 +260,7 @@ private static void AddExceptionTags(Exception exception, Activity activity, out if (exception is WebException wexc && wexc.Response is HttpWebResponse response) { statusCode = response.StatusCode; + protocolVersion = response.ProtocolVersion; if (tracingEmitOldAttributes) { @@ -267,6 +269,7 @@ private static void AddExceptionTags(Exception exception, Activity activity, out if (tracingEmitNewAttributes) { + activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(protocolVersion)); activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, (int)statusCode); } @@ -397,6 +400,7 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC { HttpStatusCode? httpStatusCode = null; string errorType = null; + Version protocolVersion = null; // Activity may be null if we are not tracing in these cases: // 1. No listeners @@ -415,11 +419,12 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC errorType = GetErrorType(ex); if (activity != null) { - AddExceptionTags(ex, activity, out httpStatusCode); + AddExceptionTags(ex, activity, out httpStatusCode, out protocolVersion); } else if (ex is WebException wexc && wexc.Response is HttpWebResponse response) { httpStatusCode = response.StatusCode; + protocolVersion = response.ProtocolVersion; } } else @@ -447,6 +452,7 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC } httpStatusCode = responseCopy.StatusCode; + protocolVersion = responseCopy.ProtocolVersion; } else { @@ -456,6 +462,7 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC } httpStatusCode = response.StatusCode; + protocolVersion = response.ProtocolVersion; } if (SpanHelper.ResolveSpanStatusForHttpStatusCode(ActivityKind.Client, (int)httpStatusCode.Value) == ActivityStatusCode.Error) @@ -531,7 +538,11 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC tags.Add(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); tags.Add(SemanticConventions.AttributeUrlScheme, request.RequestUri.Scheme); - tags.Add(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(request.ProtocolVersion)); + if (protocolVersion != null) + { + tags.Add(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(protocolVersion)); + } + if (!request.RequestUri.IsDefaultPort) { tags.Add(SemanticConventions.AttributeServerPort, request.RequestUri.Port); diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index ec7b4aac2ee..dfa89738811 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -339,13 +339,13 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( var normalizedAttributes = activity.TagObjects.Where(kv => !kv.Key.StartsWith("otel.")).ToDictionary(x => x.Key, x => x.Value.ToString()); - int numberOfNewTags = activity.Status == ActivityStatusCode.Error ? 6 : 5; - int numberOfDupeTags = activity.Status == ActivityStatusCode.Error ? 12 : 11; + int numberOfNewTags = activity.Status == ActivityStatusCode.Error ? 5 : 4; + int numberOfDupeTags = activity.Status == ActivityStatusCode.Error ? 11 : 10; var expectedAttributeCount = semanticConvention == HttpSemanticConvention.Dupe - ? numberOfDupeTags + (tc.ResponseExpected ? 2 : 0) + ? numberOfDupeTags + (tc.ResponseExpected ? 3 : 0) : semanticConvention == HttpSemanticConvention.New - ? numberOfNewTags + (tc.ResponseExpected ? 1 : 0) + ? numberOfNewTags + (tc.ResponseExpected ? 2 : 0) : 6 + (tc.ResponseExpected ? 1 : 0); Assert.Equal(expectedAttributeCount, normalizedAttributes.Count); @@ -374,9 +374,9 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeServerAddress && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerName]); Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeServerPort && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerPort]); Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeUrlFull && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpUrl]); - Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeNetworkProtocolVersion && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpFlavor]); if (tc.ResponseExpected) { + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeNetworkProtocolVersion && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpFlavor]); Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpResponseStatusCode && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); if (tc.ResponseCode >= 400) @@ -387,6 +387,7 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( else { Assert.DoesNotContain(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpResponseStatusCode); + Assert.DoesNotContain(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeNetworkProtocolVersion); #if NET8_0_OR_GREATER // we are using fake address so it will be "name_resolution_error" @@ -532,20 +533,18 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( attributes[tag.Key] = tag.Value; } -#if !NET8_0_OR_GREATER - var numberOfTags = 6; -#else - // network.protocol.version is not emitted when response if not received. - // https://github.com/open-telemetry/opentelemetry-dotnet/issues/4928 - var numberOfTags = 5; -#endif + var numberOfTags = 4; if (tc.ResponseExpected) { var expectedStatusCode = int.Parse(normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); - numberOfTags = (expectedStatusCode >= 400) ? 6 : 5; + numberOfTags = (expectedStatusCode >= 400) ? 5 : 4; // error.type extra tag + } + else + { + numberOfTags = 5; // error.type would be extra } - var expectedAttributeCount = numberOfTags + (tc.ResponseExpected ? 1 : 0); + var expectedAttributeCount = numberOfTags + (tc.ResponseExpected ? 2 : 0); // responsecode + protocolversion Assert.Equal(expectedAttributeCount, attributes.Count); @@ -553,12 +552,10 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeServerAddress && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerName]); Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeServerPort && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerPort]); Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeUrlScheme && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpScheme]); -#if !NET8_0_OR_GREATER - Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeNetworkProtocolVersion && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpFlavor]); -#endif if (tc.ResponseExpected) { + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeNetworkProtocolVersion && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpFlavor]); Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpResponseStatusCode && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); if (tc.ResponseCode >= 400) @@ -568,6 +565,7 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( } else { + Assert.DoesNotContain(attributes, kvp => kvp.Key == SemanticConventions.AttributeNetworkProtocolVersion); Assert.DoesNotContain(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpResponseStatusCode); #if NET8_0_OR_GREATER