From 5a0366146b5874053e01bc068ac5b03f5c4089b2 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Fri, 3 Nov 2023 15:31:01 -0700 Subject: [PATCH 1/4] remove status description and update unit tests --- .../Implementation/HttpInListener.cs | 2 +- .../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 - 11 files changed, 22 insertions(+), 83 deletions(-) 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/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index b5d4c85fc5e..f1773c223c6 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -344,7 +344,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 314ff67ea41..e33362d78b9 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 833e26098bb..a06218125ef 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": { From b981ebfe1b500bcdaf9b9175544c2bcb74a5a168 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Mon, 6 Nov 2023 17:07:55 -0800 Subject: [PATCH 2/4] update changelog --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 7 +++++++ src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 4091d5b9f1f..d6e40cf070a 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,6 +2,13 @@ ## Unreleased +* On Exception, Activity Status is set to Error. The Exception Message + has been removed from Activity Status Description to comply with the + [Http spec](https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md#status). + 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/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 4b7bbb8cd3e..79ee9ccb5dd 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -2,6 +2,13 @@ ## Unreleased +* On Exception, Activity Status is set to Error. The Exception Message + has been removed from Activity Status Description to comply with the + [Http spec](https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md#status). + 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) From be210413b90eaba8b5d35b7f63fb983f391fc412 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 7 Nov 2023 15:26:15 -0800 Subject: [PATCH 3/4] updated changelog --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 5 ++--- src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index d6e40cf070a..4ade5e0b3f3 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,9 +2,8 @@ ## Unreleased -* On Exception, Activity Status is set to Error. The Exception Message - has been removed from Activity Status Description to comply with the - [Http spec](https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md#status). +* Removed the Activity Status Description that was being set during unhandled + 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)) diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 79ee9ccb5dd..7594e758b17 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -2,9 +2,8 @@ ## Unreleased -* On Exception, Activity Status is set to Error. The Exception Message - has been removed from Activity Status Description to comply with the - [Http spec](https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md#status). +* Removed the Activity Status Description that was being set during unhandled + 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)) From 335f9bb9160ccb886c1898d0681c14e9ff43c90a Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 7 Nov 2023 15:34:06 -0800 Subject: [PATCH 4/4] update changelog --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 2 +- src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 4ade5e0b3f3..b259c980e9d 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -* Removed the Activity Status Description that was being set during unhandled +* 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. diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 7594e758b17..4ef52f043ff 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -* Removed the Activity Status Description that was being set during unhandled +* 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.