Skip to content

[HttpClient & HttpWebRequest] Set network protocol version from response object #5043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
utpilla merged 11 commits into from
Nov 15, 2023
Merged

[HttpClient & HttpWebRequest] Set network protocol version from response object #5043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
60ec1d6
draft
Nov 10, 2023
3b3db93
Merge branch 'main' into vibankwa/fix-network-protocol-version
Nov 10, 2023
9e4a319
fix version string
Nov 10, 2023
a444be0
fix webrequest
Nov 10, 2023
139ae33
Merge branch 'main' into vibankwa/fix-network-protocol-version
vishweshbankwar Nov 10, 2023
e9af64c
add changelog
Nov 10, 2023
ce15b7d
Merge branch 'vibankwa/fix-network-protocol-version' of https://githu…
Nov 11, 2023
fa3f20a
Merge branch 'main' into vibankwa/fix-network-protocol-version
vishweshbankwar Nov 14, 2023
3801d65
Merge branch 'main' into vibankwa/fix-network-protocol-version
vishweshbankwar Nov 14, 2023
96d605d
Merge branch 'main' into vibankwa/fix-network-protocol-version
vishweshbankwar Nov 15, 2023
0399ac1
Merge branch 'main' into vibankwa/fix-network-protocol-version
utpilla Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down Expand Up @@ -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)
{
Expand Down
2 changes: 1 addition & 1 deletion ...OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ public void OnStopEventWritten(Activity activity, object payload)

tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeServerAddress, request.RequestUri.Host));
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeUrlScheme, request.RequestUri.Scheme));
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(request.Version)));

if (!request.RequestUri.IsDefaultPort)
{
Expand All @@ -139,6 +138,7 @@ public void OnStopEventWritten(Activity activity, object payload)

if (TryFetchResponse(payload, out HttpResponseMessage response))
{
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(response.Version)));
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)));

// Set error.type to status code for failed requests
Expand Down
19 changes: 15 additions & 4 deletions src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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));
}

Expand Down Expand Up @@ -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)
{
Expand All @@ -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)
{
Expand All @@ -267,6 +269,7 @@ private static void AddExceptionTags(Exception exception, Activity activity, out

if (tracingEmitNewAttributes)
{
activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetProtocolVersionString(protocolVersion));
Copy link

@TimothyMothra TimothyMothra Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be set twice?
It's set here (line 272) in the AddExceptionTags and also the out parameter is used to set it again at Line 543.

It's also odd to me that it's being set in the AddExceptionTags helper method. This may be a candidate for refactor in the future.

Copy link
Member Author

@vishweshbankwar vishweshbankwar Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 543 sets it on metric.

activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, (int)statusCode);
}

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -447,6 +452,7 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC
}

httpStatusCode = responseCopy.StatusCode;
protocolVersion = responseCopy.ProtocolVersion;
}
else
{
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand Down
32 changes: 15 additions & 17 deletions test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand All @@ -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"
Expand Down Expand Up @@ -532,33 +533,29 @@ 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);

Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpRequestMethod && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpMethod]);
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)
Expand All @@ -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
Expand Down