AspNetCore/Http Instrumentation to leverage native Activity Status. (#…

…3555)
open-telemetry · Aug 10, 2022 · dd88099 · dd88099
1 parent d6e4d98
commit dd88099
Show file tree

Hide file tree

Showing 12 changed files with 72 additions and 60 deletions.
diff --git a/src/OpenTelemetry.Api/Internal/SpanHelper.cs b/src/OpenTelemetry.Api/Internal/SpanHelper.cs
@@ -30,15 +30,15 @@ internal static class SpanHelper
         /// <param name="kind">The span kind.</param>
         /// <param name="httpStatusCode">Http status code.</param>
         /// <returns>Resolved span <see cref="Status"/> for the Http status code.</returns>
-        public static Status ResolveSpanStatusForHttpStatusCode(ActivityKind kind, int httpStatusCode)
+        public static ActivityStatusCode ResolveSpanStatusForHttpStatusCode(ActivityKind kind, int httpStatusCode)
         {
             var upperBound = kind == ActivityKind.Client ? 399 : 499;
             if (httpStatusCode >= 100 && httpStatusCode <= upperBound)
             {
-                return Status.Unset;
+                return ActivityStatusCode.Unset;
             }
 
-            return Status.Error;
+            return ActivityStatusCode.Error;
         }
     }
 }
diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+* Updated to use Activity native support from `System.Diagnostics.DiagnosticSource`
+  to set activity status.
+  ([#3118](https://github.com/open-telemetry/opentelemetry-dotnet/issues/3118))
+  ([#3555](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3555))
+
 ## 1.0.0-rc9.5
 
 Released 2022-Aug-02

diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
@@ -207,12 +207,12 @@ public override void OnStopActivity(Activity activity, object payload)
                 {
                     AddGrpcAttributes(activity, grpcMethod, context);
                 }
-                else if (activity.GetStatus().StatusCode == StatusCode.Unset)
+                else if (activity.Status == ActivityStatusCode.Unset)
                 {
                     activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, response.StatusCode));
                 }
 #else
-                if (activity.GetStatus().StatusCode == StatusCode.Unset)
+                if (activity.Status == ActivityStatusCode.Unset)
                 {
                     activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, response.StatusCode));
                 }
@@ -298,7 +298,7 @@ public override void OnException(Activity activity, object payload)
                     activity.RecordException(exc);
                 }
 
-                activity.SetStatus(Status.Error.WithDescription(exc.Message));
+                activity.SetStatus(ActivityStatusCode.Error, exc.Message);
 
                 try
                 {

diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+* Updated to use Activity native support from `System.Diagnostics.DiagnosticSource`
+  to set activity status.
+  ([#3118](https://github.com/open-telemetry/opentelemetry-dotnet/issues/3118))
+  ([#3555](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3555))
+
 * Changed activity source name from `OpenTelemetry.HttpWebRequest`
   to `OpenTelemetry.Instrumentation.Http.HttpWebRequest` for `HttpWebRequest`s
   and from `OpenTelemetry.Instrumentation.Http`

diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
@@ -165,22 +165,22 @@ public override void OnStopActivity(Activity activity, object payload)
                 // requestTaskStatus is not null
                 _ = this.stopRequestStatusFetcher.TryFetch(payload, out var requestTaskStatus);
 
-                StatusCode currentStatusCode = activity.GetStatus().StatusCode;
+                ActivityStatusCode currentStatusCode = activity.Status;
                 if (requestTaskStatus != TaskStatus.RanToCompletion)
                 {
                     if (requestTaskStatus == TaskStatus.Canceled)
                     {
-                        if (currentStatusCode == StatusCode.Unset)
+                        if (currentStatusCode == ActivityStatusCode.Unset)
                         {
-                            activity.SetStatus(Status.Error);
+                            activity.SetStatus(ActivityStatusCode.Error);
                         }
                     }
                     else if (requestTaskStatus != TaskStatus.Faulted)
                     {
-                        if (currentStatusCode == StatusCode.Unset)
+                        if (currentStatusCode == ActivityStatusCode.Unset)
                         {
                             // Faults are handled in OnException and should already have a span.Status of Error w/ Description.
-                            activity.SetStatus(Status.Error);
+                            activity.SetStatus(ActivityStatusCode.Error);
                         }
                     }
                 }
@@ -189,7 +189,7 @@ public override void OnStopActivity(Activity activity, object payload)
                 {
                     activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode);
 
-                    if (currentStatusCode == StatusCode.Unset)
+                    if (currentStatusCode == ActivityStatusCode.Unset)
                     {
                         activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)response.StatusCode));
                     }
@@ -232,7 +232,7 @@ public override void OnException(Activity activity, object payload)
 
                 if (exc is HttpRequestException)
                 {
-                    activity.SetStatus(Status.Error.WithDescription(exc.Message));
+                    activity.SetStatus(ActivityStatusCode.Error, exc.Message);
                 }
 
                 try

diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs
@@ -141,7 +141,9 @@ private static void AddExceptionTags(Exception exception, Activity activity)
                 return;
             }
 
-            Status status;
+            ActivityStatusCode status;
+            string exceptionMessage = null;
+
             if (exception is WebException wexc)
             {
                 if (wexc.Response is HttpWebResponse response)
@@ -156,28 +158,31 @@ private static void AddExceptionTags(Exception exception, Activity activity)
                     {
                         case WebExceptionStatus.Timeout:
                         case WebExceptionStatus.RequestCanceled:
-                            status = Status.Error;
+                            status = ActivityStatusCode.Error;
                             break;
                         case WebExceptionStatus.SendFailure:
                         case WebExceptionStatus.ConnectFailure:
                         case WebExceptionStatus.SecureChannelFailure:
                         case WebExceptionStatus.TrustFailure:
                         case WebExceptionStatus.ServerProtocolViolation:
                         case WebExceptionStatus.MessageLengthLimitExceeded:
-                            status = Status.Error.WithDescription(exception.Message);
+                            status = ActivityStatusCode.Error;
+                            exceptionMessage = exception.Message;
                             break;
                         default:
-                            status = Status.Error.WithDescription(exception.Message);
+                            status = ActivityStatusCode.Error;
+                            exceptionMessage = exception.Message;
                             break;
                     }
                 }
             }
             else
             {
-                status = Status.Error.WithDescription(exception.Message);
+                status = ActivityStatusCode.Error;
+                exceptionMessage = exception.Message;
             }
 
-            activity.SetStatus(status);
+            activity.SetStatus(status, exceptionMessage);
             if (Options.RecordException)
             {
                 activity.RecordException(exception);

diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
@@ -86,10 +86,7 @@ void ConfigureTestServices(IServiceCollection services)
             var activity = exportedItems[0];
 
             Assert.Equal(200, activity.GetTagValue(SemanticConventions.AttributeHttpStatusCode));
-
-            var status = activity.GetStatus();
-            Assert.Equal(status, Status.Unset);
-
+            Assert.Equal(ActivityStatusCode.Unset, activity.Status);
             ValidateAspNetCoreActivity(activity, "/api/values");
         }
 

diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/InProcServerTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/InProcServerTests.cs
@@ -75,6 +75,8 @@ public async void ExampleTest()
             Assert.Equal("GET", activity.GetTagValue(SemanticConventions.AttributeHttpMethod));
             Assert.Equal("1.1", activity.GetTagValue(SemanticConventions.AttributeHttpFlavor));
             Assert.Equal(200, activity.GetTagValue(SemanticConventions.AttributeHttpStatusCode));
+            Assert.True(activity.Status == ActivityStatusCode.Unset);
+            Assert.True(activity.StatusDescription is null);
         }
 
         public async void Dispose()

diff --git a/....Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs b/....Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs
@@ -115,22 +115,22 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan(
 
             if (statusCode == 503)
             {
-                Assert.Equal(Status.Error.StatusCode, activity.GetStatus().StatusCode);
+                Assert.Equal(ActivityStatusCode.Error, activity.Status);
             }
             else
             {
-                Assert.Equal(Status.Unset, activity.GetStatus());
+                Assert.Equal(ActivityStatusCode.Unset, activity.Status);
             }
 
             // 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.GetStatus().Description));
+                Assert.True(string.IsNullOrEmpty(activity.StatusDescription));
             }
             else
             {
-                Assert.Equal("exception description", activity.GetStatus().Description);
+                Assert.Equal("exception description", activity.StatusDescription);
             }
 
             if (recordException)

diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs
@@ -110,13 +110,11 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut
             Assert.Equal(tc.SpanName, activity.DisplayName);
 
             // Assert.Equal(tc.SpanStatus, d[span.Status.CanonicalCode]);
-            Assert.Equal(
-                    tc.SpanStatus,
-                    activity.GetTagValue(SpanAttributeConstants.StatusCodeKey) as string);
+            Assert.Equal(tc.SpanStatus, activity.Status.ToString());
 
             if (tc.SpanStatusHasDescription.HasValue)
             {
-                var desc = activity.GetTagValue(SpanAttributeConstants.StatusDescriptionKey) as string;
+                var desc = activity.StatusDescription;
                 Assert.Equal(tc.SpanStatusHasDescription.Value, !string.IsNullOrEmpty(desc));
             }
 
@@ -194,7 +192,7 @@ public async Task DebugIndividualTestAsync()
                     ""responseCode"": 399,
                     ""responseExpected"": true,
                     ""spanName"": ""HTTP GET"",
-                    ""spanStatus"": ""UNSET"",
+                    ""spanStatus"": ""Unset"",
                     ""spanKind"": ""Client"",
                     ""spanAttributes"": {
                       ""http.scheme"": ""http"",

diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs
@@ -544,8 +544,8 @@ public async Task TestRequestWithException(string method)
             Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair<string, Activity> exceptionEvent));
             Assert.Equal("Stop", exceptionEvent.Key);
 
-            Assert.NotNull(activity.GetTagValue(SpanAttributeConstants.StatusCodeKey));
-            Assert.NotNull(activity.GetTagValue(SpanAttributeConstants.StatusDescriptionKey));
+            Assert.True(activity.Status != ActivityStatusCode.Unset);
+            Assert.NotNull(activity.StatusDescription);
         }
 
         /// <summary>
@@ -583,8 +583,8 @@ public async Task TestCanceledRequest(string method)
             Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair<string, Activity> exceptionEvent));
             Assert.Equal("Stop", exceptionEvent.Key);
 
-            Assert.NotNull(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusCodeKey));
-            Assert.Null(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusDescriptionKey));
+            Assert.True(exceptionEvent.Value.Status != ActivityStatusCode.Unset);
+            Assert.True(exceptionEvent.Value.StatusDescription == null);
         }
 
         /// <summary>
@@ -622,8 +622,8 @@ public async Task TestSecureTransportFailureRequest(string method)
             Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair<string, Activity> exceptionEvent));
             Assert.Equal("Stop", exceptionEvent.Key);
 
-            Assert.NotNull(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusCodeKey));
-            Assert.NotNull(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusDescriptionKey));
+            Assert.True(exceptionEvent.Value.Status != ActivityStatusCode.Unset);
+            Assert.NotNull(exceptionEvent.Value.StatusDescription);
         }
 
         /// <summary>
@@ -664,8 +664,8 @@ public async Task TestSecureTransportRetryFailureRequest(string method)
             Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair<string, Activity> exceptionEvent));
             Assert.Equal("Stop", exceptionEvent.Key);
 
-            Assert.NotNull(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusCodeKey));
-            Assert.NotNull(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusDescriptionKey));
+            Assert.True(exceptionEvent.Value.Status != ActivityStatusCode.Unset);
+            Assert.NotNull(exceptionEvent.Value.StatusDescription);
         }
 
         [Fact]