From 91ed41fcd972eef227eb7ac7ab70bdefd62f9c3f Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Wed, 8 Nov 2023 14:47:32 -0800 Subject: [PATCH 1/3] Update SIG meeting time to 9AM (#5031) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 996ec89fd0d..c8b3915f23a 100644 --- a/README.md +++ b/README.md @@ -98,7 +98,7 @@ extension scenarios: See [CONTRIBUTING.md](CONTRIBUTING.md) -We meet weekly on Tuesdays, and the time of the meeting alternates between 11AM +We meet weekly on Tuesdays, and the time of the meeting alternates between 9AM PT and 4PM PT. The meeting is subject to change depending on contributors' availability. Check the [OpenTelemetry community calendar](https://calendar.google.com/calendar/embed?src=google.com_b79e3e90j7bbsa2n2p5an5lf60%40group.calendar.google.com) From 4a3c8d36b3d1dda97d0f3e0721e502619ddfe83a Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Wed, 8 Nov 2023 15:50:42 -0800 Subject: [PATCH 2/3] [HttpClient] Add `error.type` for traces and metrics (#5005) Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Co-authored-by: Timothy Mothra Co-authored-by: Mikel Blanchard --- .../CHANGELOG.md | 16 ++ .../HttpHandlerDiagnosticListener.cs | 48 ++++- .../HttpHandlerMetricsDiagnosticListener.cs | 190 +++++++++++++----- .../HttpClientTests.cs | 82 +++++++- .../http-out-test-cases.json | 18 -- 5 files changed, 273 insertions(+), 81 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 4b7bbb8cd3e..bc19fe9c4fb 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -18,6 +18,22 @@ `http` or `http/dup`. ([#5003](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5003)) +* An additional attribute `error.type` will be added to activity and + `http.client.request.duration` metric in case of failed requests as per the + [specification](https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md#common-attributes). + + Users moving to `net8.0` or newer frameworks from lower versions will see + difference in values in case of an exception. `net8.0` or newer frameworks add + the ability to further drill down the exceptions to a specific type through + [HttpRequestError](https://learn.microsoft.com/dotnet/api/system.net.http.httprequesterror?view=net-8.0) + enum. For lower versions, the individual types will be rolled in to a single + type. This could be a **breaking change** if alerts are set based on the values. + + The attribute will only be added when `OTEL_SEMCONV_STABILITY_OPT_IN` + environment variable is set to `http` or `http/dup`. + + ([#5005](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5005)) + ## 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 b5d4c85fc5e..c9234e4cdcc 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -271,6 +271,11 @@ public void OnStopActivity(Activity activity, object payload) if (TryFetchResponse(payload, out HttpResponseMessage response)) { + if (currentStatusCode == ActivityStatusCode.Unset) + { + activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)response.StatusCode)); + } + if (this.emitOldAttributes) { activity.SetTag(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); @@ -279,11 +284,10 @@ public void OnStopActivity(Activity activity, object payload) if (this.emitNewAttributes) { activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); - } - - if (currentStatusCode == ActivityStatusCode.Unset) - { - activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)response.StatusCode)); + if (activity.Status == ActivityStatusCode.Error) + { + activity.SetTag(SemanticConventions.AttributeErrorType, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); + } } try @@ -337,6 +341,11 @@ public void OnException(Activity activity, object payload) return; } + if (this.emitNewAttributes) + { + activity.SetTag(SemanticConventions.AttributeErrorType, GetErrorType(exc)); + } + if (this.options.RecordException) { activity.RecordException(exc); @@ -372,4 +381,33 @@ static bool TryFetchException(object payload, out Exception exc) return true; } } + + private static string GetErrorType(Exception exc) + { +#if NET8_0_OR_GREATER + // For net8.0 and above exception type can be found using HttpRequestError. + // https://learn.microsoft.com/dotnet/api/system.net.http.httprequesterror?view=net-8.0 + if (exc is HttpRequestException httpRequestException) + { + return httpRequestException.HttpRequestError switch + { + HttpRequestError.NameResolutionError => "name_resolution_error", + HttpRequestError.ConnectionError => "connection_error", + HttpRequestError.SecureConnectionError => "secure_connection_error", + HttpRequestError.HttpProtocolError => "http_protocol_error", + HttpRequestError.ExtendedConnectNotSupported => "extended_connect_not_supported", + HttpRequestError.VersionNegotiationError => "version_negotiation_error", + HttpRequestError.UserAuthenticationError => "user_authentication_error", + HttpRequestError.ProxyTunnelError => "proxy_tunnel_error", + HttpRequestError.InvalidResponse => "invalid_response", + HttpRequestError.ResponseEnded => "response_ended", + HttpRequestError.ConfigurationLimitExceeded => "configuration_limit_exceeded", + + // Fall back to the exception type name in case of HttpRequestError.Unknown + _ => exc.GetType().FullName, + }; + } +#endif + return exc.GetType().FullName; + } } diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs index eab4c6d4dc4..48e5c59320e 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs @@ -37,11 +37,18 @@ internal sealed class HttpHandlerMetricsDiagnosticListener : ListenerHandler internal static readonly string MeterName = AssemblyName.Name; internal static readonly string MeterVersion = AssemblyName.Version.ToString(); internal static readonly Meter Meter = new(MeterName, MeterVersion); + private const string OnUnhandledExceptionEvent = "System.Net.Http.Exception"; private static readonly Histogram HttpClientDuration = Meter.CreateHistogram("http.client.duration", "ms", "Measures the duration of outbound HTTP requests."); private static readonly Histogram HttpClientRequestDuration = Meter.CreateHistogram("http.client.request.duration", "s", "Duration of HTTP client requests."); private static readonly PropertyFetcher StopRequestFetcher = new("Request"); private static readonly PropertyFetcher StopResponseFetcher = new("Response"); + private static readonly PropertyFetcher StopExceptionFetcher = new("Exception"); + private static readonly PropertyFetcher RequestFetcher = new("Request"); +#if NET6_0_OR_GREATER + private static readonly HttpRequestOptionsKey HttpRequestOptionsErrorKey = new HttpRequestOptionsKey(SemanticConventions.AttributeErrorType); +#endif + private readonly HttpClientMetricInstrumentationOptions options; private readonly bool emitOldAttributes; private readonly bool emitNewAttributes; @@ -57,84 +64,118 @@ public HttpHandlerMetricsDiagnosticListener(string name, HttpClientMetricInstrum public override void OnEventWritten(string name, object payload) { - if (name == OnStopEvent) + if (name == OnUnhandledExceptionEvent) { - if (Sdk.SuppressInstrumentation) + if (this.emitNewAttributes) { - return; + this.OnExceptionEventWritten(Activity.Current, payload); } + } + else if (name == OnStopEvent) + { + this.OnStopEventWritten(Activity.Current, payload); + } + } - var activity = Activity.Current; - if (TryFetchRequest(payload, out HttpRequestMessage request)) + public void OnStopEventWritten(Activity activity, object payload) + { + if (Sdk.SuppressInstrumentation) + { + return; + } + + if (TryFetchRequest(payload, out HttpRequestMessage request)) + { + // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md + if (this.emitOldAttributes) { - // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md - if (this.emitOldAttributes) + TagList tags = default; + + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, HttpTagHelper.GetNameForHttpMethod(request.Method))); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version))); + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host)); + + if (!request.RequestUri.IsDefaultPort) { - TagList tags = default; + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port)); + } - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, HttpTagHelper.GetNameForHttpMethod(request.Method))); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version))); - tags.Add(new KeyValuePair(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host)); + if (TryFetchResponse(payload, out HttpResponseMessage response)) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode))); + } - if (!request.RequestUri.IsDefaultPort) - { - tags.Add(new KeyValuePair(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port)); - } + // 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. + HttpClientDuration.Record(activity.Duration.TotalMilliseconds, tags); + } - if (TryFetchResponse(payload, out HttpResponseMessage response)) - { - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode))); - } + // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md + if (this.emitNewAttributes) + { + TagList tags = default; - // 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. - HttpClientDuration.Record(activity.Duration.TotalMilliseconds, tags); + if (RequestMethodHelper.KnownMethods.TryGetValue(request.Method.Method, out var httpMethod)) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, httpMethod)); } - - // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md - if (this.emitNewAttributes) + else { - TagList tags = default; + // Set to default "_OTHER" as per spec. + // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, "_OTHER")); + } - if (RequestMethodHelper.KnownMethods.TryGetValue(request.Method.Method, out var httpMethod)) - { - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, httpMethod)); - } - else - { - // Set to default "_OTHER" as per spec. - // https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#common-attributes - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, "_OTHER")); - } + 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.GetFlavorTagValueFromProtocolVersion(request.Version))); - tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version))); - tags.Add(new KeyValuePair(SemanticConventions.AttributeServerAddress, request.RequestUri.Host)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, request.RequestUri.Scheme)); + if (!request.RequestUri.IsDefaultPort) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeServerPort, request.RequestUri.Port)); + } + + if (TryFetchResponse(payload, out HttpResponseMessage response)) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode))); - if (!request.RequestUri.IsDefaultPort) + // Set error.type to status code for failed requests + // https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md#common-attributes + if (SpanHelper.ResolveSpanStatusForHttpStatusCode(ActivityKind.Client, (int)response.StatusCode) == ActivityStatusCode.Error) { - tags.Add(new KeyValuePair(SemanticConventions.AttributeServerPort, request.RequestUri.Port)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeErrorType, TelemetryHelper.GetBoxedStatusCode(response.StatusCode))); } + } - if (TryFetchResponse(payload, out HttpResponseMessage response)) + if (response == null) + { +#if !NET6_0_OR_GREATER + request.Properties.TryGetValue(SemanticConventions.AttributeErrorType, out var errorType); +#else + request.Options.TryGetValue(HttpRequestOptionsErrorKey, out var errorType); +#endif + + // Set error.type to exception type if response was not received. + // https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md#common-attributes + if (errorType != null) { - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode))); + tags.Add(new KeyValuePair(SemanticConventions.AttributeErrorType, errorType)); } - - // 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. - HttpClientRequestDuration.Record(activity.Duration.TotalSeconds, tags); } + + // 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. + HttpClientRequestDuration.Record(activity.Duration.TotalSeconds, tags); } } // The AOT-annotation DynamicallyAccessedMembers in System.Net.Http library ensures that top-level properties on the payload object are always preserved. // see https://github.com/dotnet/runtime/blob/f9246538e3d49b90b0e9128d7b1defef57cd6911/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L325 #if NET6_0_OR_GREATER - [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "The event source guarantees that top-level properties are preserved")] + [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "The System.Net.Http library guarantees that top-level properties are preserved")] #endif static bool TryFetchRequest(object payload, out HttpRequestMessage request) => StopRequestFetcher.TryFetch(payload, out request) && request != null; @@ -142,9 +183,54 @@ static bool TryFetchRequest(object payload, out HttpRequestMessage request) => // The AOT-annotation DynamicallyAccessedMembers in System.Net.Http library ensures that top-level properties on the payload object are always preserved. // see https://github.com/dotnet/runtime/blob/f9246538e3d49b90b0e9128d7b1defef57cd6911/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L325 #if NET6_0_OR_GREATER - [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "The event source guarantees that top-level properties are preserved")] + [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "The System.Net.Http library guarantees that top-level properties are preserved")] #endif static bool TryFetchResponse(object payload, out HttpResponseMessage response) => StopResponseFetcher.TryFetch(payload, out response) && response != null; } + + public void OnExceptionEventWritten(Activity activity, object payload) + { + if (!TryFetchException(payload, out Exception exc) || !TryFetchRequest(payload, out HttpRequestMessage request)) + { + HttpInstrumentationEventSource.Log.NullPayload(nameof(HttpHandlerMetricsDiagnosticListener), nameof(this.OnExceptionEventWritten)); + return; + } + +#if !NET6_0_OR_GREATER + request.Properties.Add(SemanticConventions.AttributeErrorType, exc.GetType().FullName); +#else + request.Options.Set(HttpRequestOptionsErrorKey, exc.GetType().FullName); +#endif + + // The AOT-annotation DynamicallyAccessedMembers in System.Net.Http library ensures that top-level properties on the payload object are always preserved. + // see https://github.com/dotnet/runtime/blob/f9246538e3d49b90b0e9128d7b1defef57cd6911/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L325 +#if NET6_0_OR_GREATER + [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "The System.Net.Http library guarantees that top-level properties are preserved")] +#endif + static bool TryFetchException(object payload, out Exception exc) + { + if (!StopExceptionFetcher.TryFetch(payload, out exc) || exc == null) + { + return false; + } + + return true; + } + + // The AOT-annotation DynamicallyAccessedMembers in System.Net.Http library ensures that top-level properties on the payload object are always preserved. + // see https://github.com/dotnet/runtime/blob/f9246538e3d49b90b0e9128d7b1defef57cd6911/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L325 +#if NET6_0_OR_GREATER + [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "The System.Net.Http library guarantees that top-level properties are preserved")] +#endif + static bool TryFetchRequest(object payload, out HttpRequestMessage request) + { + if (!RequestFetcher.TryFetch(payload, out request) || request == null) + { + return false; + } + + return true; + } + } } diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index 314ff67ea41..9ff51ebef41 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -51,7 +51,7 @@ await HttpOutCallsAreCollectedSuccessfullyBodyAsync( [Theory] [MemberData(nameof(TestData))] - public async Task HttpOutCallsAreCollectedSuccessfullyTracesAndMetricsNewSemanticConventionsAsync(HttpTestData.HttpOutTestCase tc) + public async Task HttpOutCallsAreCollectedSuccessfullyTracesAndMetricsDuplicateSemanticConventionsAsync(HttpTestData.HttpOutTestCase tc) { await HttpOutCallsAreCollectedSuccessfullyBodyAsync( this.host, @@ -59,12 +59,13 @@ await HttpOutCallsAreCollectedSuccessfullyBodyAsync( tc, enableTracing: true, enableMetrics: true, - semanticConvention: HttpSemanticConvention.New).ConfigureAwait(false); + semanticConvention: HttpSemanticConvention.Dupe).ConfigureAwait(false); } +#endif [Theory] [MemberData(nameof(TestData))] - public async Task HttpOutCallsAreCollectedSuccessfullyTracesAndMetricsDuplicateSemanticConventionsAsync(HttpTestData.HttpOutTestCase tc) + public async Task HttpOutCallsAreCollectedSuccessfullyTracesAndMetricsNewSemanticConventionsAsync(HttpTestData.HttpOutTestCase tc) { await HttpOutCallsAreCollectedSuccessfullyBodyAsync( this.host, @@ -72,9 +73,8 @@ await HttpOutCallsAreCollectedSuccessfullyBodyAsync( tc, enableTracing: true, enableMetrics: true, - semanticConvention: HttpSemanticConvention.Dupe).ConfigureAwait(false); + semanticConvention: HttpSemanticConvention.New).ConfigureAwait(false); } -#endif [Theory] [MemberData(nameof(TestData))] @@ -346,11 +346,22 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( var normalizedAttributes = activity.TagObjects.Where(kv => !kv.Key.StartsWith("otel.")).ToDictionary(x => x.Key, x => x.Value.ToString()); +#if !NETFRAMEWORK + int numberOfNewTags = activity.Status == ActivityStatusCode.Error ? 6 : 5; + int numberOfDupeTags = activity.Status == ActivityStatusCode.Error ? 12 : 11; + + var expectedAttributeCount = semanticConvention == HttpSemanticConvention.Dupe + ? numberOfDupeTags + (tc.ResponseExpected ? 2 : 0) + : semanticConvention == HttpSemanticConvention.New + ? numberOfNewTags + (tc.ResponseExpected ? 1 : 0) + : 6 + (tc.ResponseExpected ? 1 : 0); +#else var expectedAttributeCount = semanticConvention == HttpSemanticConvention.Dupe ? 11 + (tc.ResponseExpected ? 2 : 0) : semanticConvention == HttpSemanticConvention.New ? 5 + (tc.ResponseExpected ? 1 : 0) : 6 + (tc.ResponseExpected ? 1 : 0); +#endif Assert.Equal(expectedAttributeCount, normalizedAttributes.Count); @@ -382,10 +393,26 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( if (tc.ResponseExpected) { Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpResponseStatusCode && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); + +#if !NETFRAMEWORK + if (tc.ResponseCode >= 400) + { + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeErrorType && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); + } +#endif } else { Assert.DoesNotContain(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpResponseStatusCode); +#if !NETFRAMEWORK +#if !NET8_0_OR_GREATER + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeErrorType && kvp.Value.ToString() == "System.Net.Http.HttpRequestException"); +#else + // we are using fake address so it will be "name_resolution_error" + // TODO: test other error types. + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeErrorType && kvp.Value.ToString() == "name_resolution_error"); +#endif +#endif } } @@ -505,7 +532,9 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( if (enableTracing) { var activity = Assert.Single(activities); +#if !NET8_0_OR_GREATER Assert.Equal(activity.Duration.TotalSeconds, sum); +#endif } else { @@ -519,22 +548,63 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( attributes[tag.Key] = tag.Value; } +#if !NETFRAMEWORK +#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 + if (tc.ResponseExpected) + { + var expectedStatusCode = int.Parse(normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); + numberOfTags = (expectedStatusCode >= 400) ? 6 : 5; + } + + var expectedAttributeCount = numberOfTags + (tc.ResponseExpected ? 1 : 0); +#else var expectedAttributeCount = 5 + (tc.ResponseExpected ? 1 : 0); +#endif 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.AttributeNetworkProtocolVersion && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpFlavor]); 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.AttributeHttpResponseStatusCode && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); + +#if !NETFRAMEWORK + if (tc.ResponseCode >= 400) + { + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeErrorType && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); + } +#endif } else { Assert.DoesNotContain(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpResponseStatusCode); + +#if !NETFRAMEWORK +#if !NET8_0_OR_GREATER + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeErrorType && kvp.Value.ToString() == "System.Net.Http.HttpRequestException"); +#else + // we are using fake address so it will be "name_resolution_error" + // TODO: test other error types. + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeErrorType && kvp.Value.ToString() == "name_resolution_error"); + + // network.protocol.version is not emitted when response if not received. + // https://github.com/open-telemetry/opentelemetry-dotnet/issues/4928 + Assert.DoesNotContain(attributes, kvp => kvp.Key == SemanticConventions.AttributeNetworkProtocolVersion); +#endif +#endif } // Inspect Histogram Bounds diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json b/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json index 833e26098bb..8ebbe29ff1e 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json @@ -321,24 +321,6 @@ "http.url": "http://{host}:{port}/" } }, - { - "name": "Response code: 600", - "method": "GET", - "url": "http://{host}:{port}/", - "responseCode": 600, - "spanName": "HTTP GET", - "spanStatus": "Error", - "responseExpected": true, - "spanAttributes": { - "http.scheme": "http", - "http.method": "GET", - "net.peer.name": "{host}", - "net.peer.port": "{port}", - "http.flavor": "{flavor}", - "http.status_code": "600", - "http.url": "http://{host}:{port}/" - } - }, { "name": "Http version attribute populated", "method": "GET", From f25ff3a4f4687e7eddafdf72ce0ee0967819b503 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Date: Wed, 8 Nov 2023 17:38:55 -0800 Subject: [PATCH 3/3] [AspNetCore] [Http] remove Activity Status Description and update unit tests (#5025) --- .../CHANGELOG.md | 6 +++ .../Implementation/HttpInListener.cs | 2 +- .../CHANGELOG.md | 6 +++ .../HttpHandlerDiagnosticListener.cs | 2 +- .../HttpWebRequestActivitySource.netfx.cs | 51 +++++-------------- ...stsCollectionsIsAccordingToTheSpecTests.cs | 9 +--- ...llectionsIsAccordingToTheSpecTests_Dupe.cs | 9 +--- ...ollectionsIsAccordingToTheSpecTests_New.cs | 9 +--- .../HttpClientTests.cs | 7 +-- .../HttpTestData.cs | 2 - ...HttpWebRequestActivitySourceTests.netfx.cs | 6 +-- .../HttpWebRequestTests.cs | 6 +-- .../http-out-test-cases.json | 2 - 13 files changed, 34 insertions(+), 83 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 4091d5b9f1f..b259c980e9d 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +* Removed the Activity Status Description that was being set during + exceptions. Activity Status will continue to be reported as `Error`. + This is a **breaking change**. `EnrichWithException` can be leveraged + to restore this behavior. + ([#5025](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5025)) + * Updated `http.request.method` to match specification guidelines. * For activity, if the method does not belong to one of the [known values](https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#:~:text=http.request.method%20has%20the%20following%20list%20of%20well%2Dknown%20values) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index fbf39179482..7160ab666c2 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -435,7 +435,7 @@ public void OnException(Activity activity, object payload) activity.RecordException(exc); } - activity.SetStatus(ActivityStatusCode.Error, exc.Message); + activity.SetStatus(ActivityStatusCode.Error); try { diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index bc19fe9c4fb..25bdfb76a51 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +* Removed the Activity Status Description that was being set during + exceptions. Activity Status will continue to be reported as `Error`. + This is a **breaking change**. `EnrichWithException` can be leveraged + to restore this behavior. + ([#5025](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5025)) + * Updated `http.request.method` to match specification guidelines. * For activity, if the method does not belong to one of the [known values](https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#:~:text=http.request.method%20has%20the%20following%20list%20of%20well%2Dknown%20values) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index c9234e4cdcc..f8fe389a943 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -353,7 +353,7 @@ public void OnException(Activity activity, object payload) if (exc is HttpRequestException) { - activity.SetStatus(ActivityStatusCode.Error, exc.Message); + activity.SetStatus(ActivityStatusCode.Error); } try diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index d2c23f6c6bf..095ace34d67 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -230,57 +230,30 @@ private static void AddExceptionTags(Exception exception, Activity activity, out } ActivityStatusCode status; - string exceptionMessage = null; - if (exception is WebException wexc) + if (exception is WebException wexc && wexc.Response is HttpWebResponse response) { - if (wexc.Response is HttpWebResponse response) - { - statusCode = response.StatusCode; - - if (tracingEmitOldAttributes) - { - activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)statusCode); - } + statusCode = response.StatusCode; - if (tracingEmitNewAttributes) - { - activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, (int)statusCode); - } - - status = SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)statusCode); + if (tracingEmitOldAttributes) + { + activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)statusCode); } - else + + if (tracingEmitNewAttributes) { - switch (wexc.Status) - { - case WebExceptionStatus.Timeout: - case WebExceptionStatus.RequestCanceled: - status = ActivityStatusCode.Error; - break; - case WebExceptionStatus.SendFailure: - case WebExceptionStatus.ConnectFailure: - case WebExceptionStatus.SecureChannelFailure: - case WebExceptionStatus.TrustFailure: - case WebExceptionStatus.ServerProtocolViolation: - case WebExceptionStatus.MessageLengthLimitExceeded: - status = ActivityStatusCode.Error; - exceptionMessage = exception.Message; - break; - default: - status = ActivityStatusCode.Error; - exceptionMessage = exception.Message; - break; - } + activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, (int)statusCode); } + + status = SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)statusCode); } else { status = ActivityStatusCode.Error; - exceptionMessage = exception.Message; } - activity.SetStatus(status, exceptionMessage); + activity.SetStatus(status); + if (TracingOptions.RecordException) { activity.RecordException(exception); diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs index 58688301760..1059bee9281 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs @@ -131,14 +131,7 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan_Old( // Instrumentation is not expected to set status description // as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode - if (!urlPath.EndsWith("exception")) - { - Assert.True(string.IsNullOrEmpty(activity.StatusDescription)); - } - else - { - Assert.Equal("exception description", activity.StatusDescription); - } + Assert.Null(activity.StatusDescription); if (recordException) { diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe.cs index 622498876cd..8bfe675ed5d 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe.cs @@ -138,14 +138,7 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan_Dupe( // Instrumentation is not expected to set status description // as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode - if (!urlPath.EndsWith("exception")) - { - Assert.True(string.IsNullOrEmpty(activity.StatusDescription)); - } - else - { - Assert.Equal("exception description", activity.StatusDescription); - } + Assert.Null(activity.StatusDescription); if (recordException) { diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs index cf66df98ee7..5c56ffb6719 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs @@ -132,14 +132,7 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan_New( // Instrumentation is not expected to set status description // as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode - if (!urlPath.EndsWith("exception")) - { - Assert.True(string.IsNullOrEmpty(activity.StatusDescription)); - } - else - { - Assert.Equal("exception description", activity.StatusDescription); - } + Assert.Null(activity.StatusDescription); if (recordException) { diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index 9ff51ebef41..ad3344d8a29 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -337,12 +337,7 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( // Assert.Equal(tc.SpanStatus, d[span.Status.CanonicalCode]); Assert.Equal(tc.SpanStatus, activity.Status.ToString()); - - if (tc.SpanStatusHasDescription.HasValue) - { - var desc = activity.StatusDescription; - Assert.Equal(tc.SpanStatusHasDescription.Value, !string.IsNullOrEmpty(desc)); - } + Assert.Null(activity.StatusDescription); var normalizedAttributes = activity.TagObjects.Where(kv => !kv.Key.StartsWith("otel.")).ToDictionary(x => x.Key, x => x.Value.ToString()); diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpTestData.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpTestData.cs index 86daf630db5..705c7c9f876 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpTestData.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpTestData.cs @@ -72,8 +72,6 @@ public class HttpOutTestCase public string SpanStatus { get; set; } - public bool? SpanStatusHasDescription { get; set; } - public Dictionary SpanAttributes { get; set; } } } diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs index 6eca1d40eed..074ae7df55c 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs @@ -549,7 +549,7 @@ public async Task TestRequestWithException(string method) Assert.Equal("Stop", exceptionEvent.Key); Assert.True(activity.Status != ActivityStatusCode.Unset); - Assert.NotNull(activity.StatusDescription); + Assert.Null(activity.StatusDescription); } /// @@ -627,7 +627,7 @@ public async Task TestSecureTransportFailureRequest(string method) Assert.Equal("Stop", exceptionEvent.Key); Assert.True(exceptionEvent.Value.Status != ActivityStatusCode.Unset); - Assert.NotNull(exceptionEvent.Value.StatusDescription); + Assert.Null(exceptionEvent.Value.StatusDescription); } /// @@ -669,7 +669,7 @@ public async Task TestSecureTransportRetryFailureRequest(string method) Assert.Equal("Stop", exceptionEvent.Key); Assert.True(exceptionEvent.Value.Status != ActivityStatusCode.Unset); - Assert.NotNull(exceptionEvent.Value.StatusDescription); + Assert.Null(exceptionEvent.Value.StatusDescription); } [Fact] diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.cs index 88e5b6e3813..44d79860889 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.cs @@ -123,11 +123,7 @@ public void HttpOutCallsAreCollectedSuccessfully(HttpTestData.HttpOutTestCase tc if (tag.Key == SpanAttributeConstants.StatusDescriptionKey) { - if (tc.SpanStatusHasDescription.HasValue) - { - Assert.Equal(tc.SpanStatusHasDescription.Value, !string.IsNullOrEmpty(tagValue)); - } - + Assert.Null(tagValue); continue; } diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json b/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json index 8ebbe29ff1e..d808d5c00f2 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json @@ -93,7 +93,6 @@ "url": "http://sdlfaldfjalkdfjlkajdflkajlsdjf:{port}/", "spanName": "HTTP GET", "spanStatus": "Error", - "spanStatusHasDescription": true, "responseExpected": false, "recordException": false, "spanAttributes": { @@ -111,7 +110,6 @@ "url": "http://sdlfaldfjalkdfjlkajdflkajlsdjf:{port}/", "spanName": "HTTP GET", "spanStatus": "Error", - "spanStatusHasDescription": true, "responseExpected": false, "recordException": true, "spanAttributes": {