Skip to content
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.

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

[HttpClient] Fix missing metric during network failures #4098

4 changes: 4 additions & 0 deletions src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

* Fixed an issue of missing `http.client.duration` metric data in case of
network failures.
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved
([#4098](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4098))

## 1.4.0-rc9.13

Released 2023-Feb-10
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ internal sealed class HttpHandlerMetricsDiagnosticListener : ListenerHandler
internal const string OnStopEvent = "System.Net.Http.HttpRequestOut.Stop";

private readonly PropertyFetcher<HttpResponseMessage> stopResponseFetcher = new("Response");
private readonly PropertyFetcher<HttpRequestMessage> stopRequestFetcher = new("Request");
private readonly Histogram<double> httpClientDuration;

public HttpHandlerMetricsDiagnosticListener(string name, Meter meter)
Expand All @@ -46,14 +47,11 @@ public override void OnEventWritten(string name, object payload)
}

var activity = Activity.Current;
if (this.stopResponseFetcher.TryFetch(payload, out HttpResponseMessage response) && response != null)
if (this.stopRequestFetcher.TryFetch(payload, out HttpRequestMessage request) && request != null)
{
var request = response.RequestMessage;

TagList tags = default;
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpMethod, HttpTagHelper.GetNameForHttpMethod(request.Method)));
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme));
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode));
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version)));
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host));

Expand All @@ -62,6 +60,11 @@ public override void OnEventWritten(string name, object payload)
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port));
}

if (this.stopResponseFetcher.TryFetch(payload, out HttpResponseMessage response) && response != null)
{
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode));
}

// We are relying here on HttpClient library to set duration before writing the stop event.
// https://github.com/dotnet/runtime/blob/90603686d314147017c8bbe1fa8965776ce607d0/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L178
// TODO: Follow up with .NET team if we can continue to rely on this behavior.
Expand Down
75 changes: 38 additions & 37 deletions test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -157,58 +157,59 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut
Assert.True(enrichWithExceptionCalled);
}

if (tc.ResponseExpected)
{
#if NETFRAMEWORK
Assert.Empty(requestMetrics);
Assert.Empty(requestMetrics);
#else
Assert.Single(requestMetrics);
Assert.Single(requestMetrics);

var metric = requestMetrics[0];
Assert.NotNull(metric);
Assert.True(metric.MetricType == MetricType.Histogram);
var metric = requestMetrics[0];
Assert.NotNull(metric);
Assert.True(metric.MetricType == MetricType.Histogram);

var metricPoints = new List<MetricPoint>();
foreach (var p in metric.GetMetricPoints())
{
metricPoints.Add(p);
}
var metricPoints = new List<MetricPoint>();
foreach (var p in metric.GetMetricPoints())
{
metricPoints.Add(p);
}

Assert.Single(metricPoints);
var metricPoint = metricPoints[0];
Assert.Single(metricPoints);
var metricPoint = metricPoints[0];

var count = metricPoint.GetHistogramCount();
var sum = metricPoint.GetHistogramSum();
var count = metricPoint.GetHistogramCount();
var sum = metricPoint.GetHistogramSum();

Assert.Equal(1L, count);
Assert.Equal(activity.Duration.TotalMilliseconds, sum);
Assert.Equal(1L, count);
Assert.Equal(activity.Duration.TotalMilliseconds, sum);

var attributes = new KeyValuePair<string, object>[metricPoint.Tags.Count];
int i = 0;
foreach (var tag in metricPoint.Tags)
{
attributes[i++] = tag;
}
var attributes = new KeyValuePair<string, object>[metricPoint.Tags.Count];
int i = 0;
foreach (var tag in metricPoint.Tags)
{
attributes[i++] = tag;
}

var method = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpMethod, tc.Method);
var scheme = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpScheme, "http");
var statusCode = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, tc.ResponseCode == 0 ? 200 : tc.ResponseCode);
var flavor = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpFlavor, "2.0");
var hostName = new KeyValuePair<string, object>(SemanticConventions.AttributeNetPeerName, host);
var portNumber = new KeyValuePair<string, object>(SemanticConventions.AttributeNetPeerPort, port);
Assert.Contains(method, attributes);
Assert.Contains(scheme, attributes);
var method = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpMethod, tc.Method);
var scheme = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpScheme, "http");
var statusCode = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, tc.ResponseCode == 0 ? 200 : tc.ResponseCode);
var flavor = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpFlavor, "2.0");
var hostName = new KeyValuePair<string, object>(SemanticConventions.AttributeNetPeerName, tc.ResponseExpected ? host : "sdlfaldfjalkdfjlkajdflkajlsdjf");
var portNumber = new KeyValuePair<string, object>(SemanticConventions.AttributeNetPeerPort, port);
Assert.Contains(hostName, attributes);
Assert.Contains(portNumber, attributes);
Assert.Contains(method, attributes);
Assert.Contains(scheme, attributes);
Assert.Contains(flavor, attributes);
if (tc.ResponseExpected)
{
Assert.Contains(statusCode, attributes);
Assert.Contains(flavor, attributes);
Assert.Contains(hostName, attributes);
Assert.Contains(portNumber, attributes);
Assert.Equal(6, attributes.Length);
#endif
}
else
{
Assert.Empty(requestMetrics);
Assert.DoesNotContain(statusCode, attributes);
Assert.Equal(5, attributes.Length);
}
#endif
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,35 +90,37 @@
{
"name": "Call that cannot resolve DNS will be reported as error span",
"method": "GET",
"url": "https://sdlfaldfjalkdfjlkajdflkajlsdjf.sdlkjafsdjfalfadslkf.com/",
"url": "http://sdlfaldfjalkdfjlkajdflkajlsdjf:{port}/",
Copy link
Member

Choose a reason for hiding this comment

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

why changed these?

Copy link
Member Author

Choose a reason for hiding this comment

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

wanted to validate we capture port information as well.

"spanName": "HTTP GET",
"spanStatus": "Error",
"spanStatusHasDescription": true,
"responseExpected": false,
"recordException": false,
"spanAttributes": {
"http.scheme": "https",
"http.scheme": "http",
"http.method": "GET",
"net.peer.name": "sdlfaldfjalkdfjlkajdflkajlsdjf.sdlkjafsdjfalfadslkf.com",
"net.peer.name": "sdlfaldfjalkdfjlkajdflkajlsdjf",
"net.peer.port": "{port}",
"http.flavor": "{flavor}",
"http.url": "https://sdlfaldfjalkdfjlkajdflkajlsdjf.sdlkjafsdjfalfadslkf.com/"
"http.url": "http://sdlfaldfjalkdfjlkajdflkajlsdjf:{port}/"
}
},
{
"name": "Call that cannot resolve DNS will be reported as error span. And Records exception",
"method": "GET",
"url": "https://sdlfaldfjalkdfjlkajdflkajlsdjf.sdlkjafsdjfalfadslkf.com/",
"url": "http://sdlfaldfjalkdfjlkajdflkajlsdjf:{port}/",
"spanName": "HTTP GET",
"spanStatus": "Error",
"spanStatusHasDescription": true,
"responseExpected": false,
"recordException": true,
"spanAttributes": {
"http.scheme": "https",
"http.scheme": "http",
"http.method": "GET",
"net.peer.name": "sdlfaldfjalkdfjlkajdflkajlsdjf.sdlkjafsdjfalfadslkf.com",
"net.peer.name": "sdlfaldfjalkdfjlkajdflkajlsdjf",
"net.peer.port": "{port}",
"http.flavor": "{flavor}",
"http.url": "https://sdlfaldfjalkdfjlkajdflkajlsdjf.sdlkjafsdjfalfadslkf.com/"
"http.url": "http://sdlfaldfjalkdfjlkajdflkajlsdjf:{port}/"
}
},
{
Expand Down