Status Improvements (#1579)

* Send StatusCode as a string. Send error flag in Zipkin & Jaeger when StatusCode == Error.

* Missed one spot.

* Removed code setting otel.status_description to http status description.

* Unit test fixup.

* Update CHANGELOG.

* Updated http instrumentation CHANGELOG.

* Code review.

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

examples/Console/TestRedis.cs

@@ -95,9 +95,7 @@ private static void DoWork(IDatabase db, ActivitySource activitySource)
                 }
                 catch (ArgumentOutOfRangeException e)
                 {
-                    // Set status upon error
-                    activity.SetTag(SpanAttributeConstants.StatusCodeKey, (int)Status.Error.StatusCode);
-                    activity.SetTag(SpanAttributeConstants.StatusDescriptionKey, e.ToString());
+                    activity.SetStatus(Status.Error.WithDescription(e.ToString()));
                 }
 
                 // Annotate our activity to capture metadata about our operation

src/OpenTelemetry.Api/CHANGELOG.md

@@ -2,6 +2,13 @@
 
 ## Unreleased
 
+* In order to align with the
+  [spec](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-status)
+  the `Status` (otel.status_code) tag (added on `Activity` using the `SetStatus`
+  extension) will now be set as the `Unset`, `Error`, or `Ok` string
+  representation instead of the `0`, `1`, or `2` integer representation.
+  ([#1579](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1579))
+
 ## 1.0.0-rc1.1
 
 Released 2020-Nov-17

src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs

@@ -14,7 +14,6 @@
 // limitations under the License.
 // </copyright>
 
-using System;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Reflection;
@@ -34,42 +33,29 @@ internal static class ActivityHelperExtensions
         /// This extension provides a workaround to retrieve Status from special tags with key name otel.status_code and otel.status_description.
         /// </summary>
         /// <param name="activity">Activity instance.</param>
-        /// <returns>Activity execution status.</returns>
+        /// <param name="statusCode"><see cref="StatusCode"/>.</param>
+        /// <param name="statusDescription">Status description.</param>
+        /// <returns><see langword="true"/> if <see cref="Status"/> was found on the supplied Activity.</returns>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1062:Validate arguments of public methods", Justification = "ActivityProcessor is hot path")]
-        public static Status GetStatusHelper(this Activity activity)
+        public static bool TryGetStatus(this Activity activity, out StatusCode statusCode, out string statusDescription)
         {
             Debug.Assert(activity != null, "Activity should not be null");
 
             ActivityStatusTagEnumerator state = default;
 
             ActivityTagsEnumeratorFactory<ActivityStatusTagEnumerator>.Enumerate(activity, ref state);
 
-            if (!state.IsValid)
+            if (!state.StatusCode.HasValue)
             {
-                return default;
+                statusCode = default;
+                statusDescription = null;
+                return false;
             }
 
-            Status status;
-            if (state.StatusCode == StatusCode.Error)
-            {
-                status = Status.Error;
-            }
-            else if (state.StatusCode == StatusCode.Ok)
-            {
-                status = Status.Ok;
-            }
-            else
-            {
-                status = Status.Unset;
-            }
-
-            if (!string.IsNullOrEmpty(state.StatusDescription))
-            {
-                return status.WithDescription(state.StatusDescription);
-            }
-
-            return status;
+            statusCode = state.StatusCode.Value;
+            statusDescription = state.StatusDescription;
+            return true;
         }
 
         /// <summary>
@@ -193,9 +179,7 @@ public bool ForEach(KeyValuePair<string, object> item)
 
         private struct ActivityStatusTagEnumerator : IActivityEnumerator<KeyValuePair<string, object>>
         {
-            public bool IsValid;
-
-            public StatusCode StatusCode;
+            public StatusCode? StatusCode;
 
             public string StatusDescription;
 
@@ -204,15 +188,14 @@ public bool ForEach(KeyValuePair<string, object> item)
                 switch (item.Key)
                 {
                     case SpanAttributeConstants.StatusCodeKey:
-                        this.StatusCode = (StatusCode)item.Value;
-                        this.IsValid = this.StatusCode == StatusCode.Error || this.StatusCode == StatusCode.Ok || this.StatusCode == StatusCode.Unset;
+                        this.StatusCode = StatusHelper.GetStatusCodeForStringName(item.Value as string);
                         break;
                     case SpanAttributeConstants.StatusDescriptionKey:
                         this.StatusDescription = item.Value as string;
                         break;
                 }
 
-                return this.IsValid || this.StatusDescription == null;
+                return !this.StatusCode.HasValue || this.StatusDescription == null;
             }
         }
 

src/OpenTelemetry.Api/Internal/SpanHelper.cs

@@ -29,14 +29,12 @@ internal static class SpanHelper
         /// <returns>Resolved span <see cref="Status"/> for the Http status code.</returns>
         public static Status ResolveSpanStatusForHttpStatusCode(int httpStatusCode)
         {
-            var status = Status.Error;
-
             if (httpStatusCode >= 100 && httpStatusCode <= 399)
             {
-                status = Status.Unset;
+                return Status.Unset;
             }
 
-            return status;
+            return Status.Error;
         }
     }
 }

src/OpenTelemetry.Api/Internal/StatusHelper.cs

@@ -0,0 +1,58 @@
+// <copyright file="StatusHelper.cs" company="OpenTelemetry Authors">
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+// </copyright>
+
+using System.Runtime.CompilerServices;
+using OpenTelemetry.Trace;
+
+namespace OpenTelemetry.Internal
+{
+    internal static class StatusHelper
+    {
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static string GetStringNameForStatusCode(StatusCode statusCode)
+        {
+            return statusCode switch
+            {
+                /*
+                 * Note: Order here does matter for perf. Unset is
+                 * first because assumption is most spans will be
+                 * Unset, then Error. Ok is not set by the SDK.
+                 */
+                StatusCode.Unset => "Unset",
+                StatusCode.Error => "Error",
+                StatusCode.Ok => "Ok",
+                _ => null,
+            };
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static StatusCode? GetStatusCodeForStringName(string statusCodeName)
+        {
+            return statusCodeName switch
+            {
+                /*
+                 * Note: Order here does matter for perf. Unset is
+                 * first because assumption is most spans will be
+                 * Unset, then Error. Ok is not set by the SDK.
+                 */
+                "Unset" => StatusCode.Unset,
+                "Error" => StatusCode.Error,
+                "Ok" => StatusCode.Ok,
+                _ => (StatusCode?)null,
+            };
+        }
+    }
+}

src/OpenTelemetry.Api/Trace/ActivityExtensions.cs

@@ -40,7 +40,7 @@ public static void SetStatus(this Activity activity, Status status)
         {
             Debug.Assert(activity != null, "Activity should not be null");
 
-            activity.SetTag(SpanAttributeConstants.StatusCodeKey, (int)status.StatusCode);
+            activity.SetTag(SpanAttributeConstants.StatusCodeKey, StatusHelper.GetStringNameForStatusCode(status.StatusCode));
             activity.SetTag(SpanAttributeConstants.StatusDescriptionKey, status.Description);
         }
 
@@ -55,7 +55,12 @@ public static void SetStatus(this Activity activity, Status status)
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1062:Validate arguments of public methods", Justification = "ActivityProcessor is hot path")]
         public static Status GetStatus(this Activity activity)
         {
-            return activity.GetStatusHelper();
+            if (!activity.TryGetStatus(out StatusCode statusCode, out string statusDescription))
+            {
+                return Status.Unset;
+            }
+
+            return new Status(statusCode, statusDescription);
         }
 
         /// <summary>

src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## Unreleased
 
+* Jaeger will now set the `error` tag when `otel.status_code` is set to `Error`.
+  ([#1579](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1579))
+
 ## 1.0.0-rc1.1
 
 Released 2020-Nov-17

src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs

@@ -255,6 +255,11 @@ private static void ProcessJaegerTag(ref TagEnumerationState state, string key,
             if (jaegerTag.VStr != null)
             {
                 PeerServiceResolver.InspectTag(ref state, key, jaegerTag.VStr);
+
+                if (key == SpanAttributeConstants.StatusCodeKey && jaegerTag.VStr == "Error")
+                {
+                    PooledList<JaegerTag>.Add(ref state.Tags, new JaegerTag("error", JaegerTagType.BOOL, vBool: true));
+                }
             }
             else if (jaegerTag.VLong.HasValue)
             {

src/OpenTelemetry.Exporter.Jaeger/OpenTelemetry.Exporter.Jaeger.csproj

@@ -15,6 +15,7 @@
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\SemanticConventions.cs" Link="Includes\SemanticConventions.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\SpanAttributeConstants.cs" Link="Includes\SpanAttributeConstants.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ActivityHelperExtensions.cs" Link="Includes\ActivityHelperExtensions.cs" />
+    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\StatusHelper.cs" Link="Includes\StatusHelper.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\IActivityEnumerator.cs" Link="Includes\IActivityEnumerator.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\EnumerationHelper.cs" Link="Includes\EnumerationHelper.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\PooledList.cs" Link="Includes\PooledList.cs" />

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ActivityExtensions.cs

@@ -239,15 +239,17 @@ internal static OtlpCommon.KeyValue ToOtlpAttribute(this KeyValuePair<string, ob
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private static OtlpTrace.Status ToOtlpStatus(ref TagEnumerationState otlpTags)
         {
-            if (!otlpTags.StatusCode.HasValue)
+            var status = StatusHelper.GetStatusCodeForStringName(otlpTags.StatusCode);
+
+            if (!status.HasValue)
             {
                 return null;
             }
 
             var otlpStatus = new OtlpTrace.Status
             {
                 // The numerical values of the two enumerations match, a simple cast is enough.
-                Code = (OtlpTrace.Status.Types.StatusCode)otlpTags.StatusCode,
+                Code = (OtlpTrace.Status.Types.StatusCode)(int)status,
             };
 
             if (otlpStatus.Code != OtlpTrace.Status.Types.StatusCode.Error)
@@ -370,7 +372,7 @@ private struct TagEnumerationState : IActivityEnumerator<KeyValuePair<string, ob
 
             public PooledList<OtlpCommon.KeyValue> Tags;
 
-            public int? StatusCode;
+            public string StatusCode;
 
             public string StatusDescription;
 
@@ -396,7 +398,7 @@ public bool ForEach(KeyValuePair<string, object> activityTag)
                 switch (key)
                 {
                     case SpanAttributeConstants.StatusCodeKey:
-                        this.StatusCode = activityTag.Value as int?;
+                        this.StatusCode = activityTag.Value as string;
                         return true;
                     case SpanAttributeConstants.StatusDescriptionKey:
                         this.StatusDescription = activityTag.Value as string;

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj

@@ -15,6 +15,7 @@
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\SemanticConventions.cs" Link="Includes\SemanticConventions.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\SpanAttributeConstants.cs" Link="Includes\SpanAttributeConstants.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ActivityHelperExtensions.cs" Link="Includes\ActivityHelperExtensions.cs" />
+    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\StatusHelper.cs" Link="Includes\StatusHelper.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\IActivityEnumerator.cs" Link="Includes\IActivityEnumerator.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\EnumerationHelper.cs" Link="Includes\EnumerationHelper.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\PooledList.cs" Link="Includes\PooledList.cs" />

src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## Unreleased
 
+* Zipkin will now set the `error` tag when `otel.status_code` is set to `Error`.
+  ([#1579](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1579))
+
 ## 1.0.0-rc1.1
 
 Released 2020-Nov-17

src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs

@@ -174,6 +174,11 @@ public bool ForEach(KeyValuePair<string, object> activityTag)
                 if (activityTag.Value is string strVal)
                 {
                     PeerServiceResolver.InspectTag(ref this, key, strVal);
+
+                    if (key == SpanAttributeConstants.StatusCodeKey && strVal == "Error")
+                    {
+                        PooledList<KeyValuePair<string, object>>.Add(ref this.Tags, new KeyValuePair<string, object>("error", "true"));
+                    }
                 }
                 else if (activityTag.Value is int intVal && activityTag.Key == SemanticConventions.AttributeNetPeerPort)
                 {

src/OpenTelemetry.Exporter.Zipkin/OpenTelemetry.Exporter.Zipkin.csproj

@@ -10,6 +10,7 @@
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\SemanticConventions.cs" Link="Includes\SemanticConventions.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\SpanAttributeConstants.cs" Link="Includes\SpanAttributeConstants.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ActivityHelperExtensions.cs" Link="Includes\ActivityHelperExtensions.cs" />
+    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\StatusHelper.cs" Link="Includes\StatusHelper.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\IActivityEnumerator.cs" Link="Includes\IActivityEnumerator.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\EnumerationHelper.cs" Link="Includes\EnumerationHelper.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\PooledList.cs" Link="Includes\PooledList.cs" />

src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs

@@ -180,8 +180,7 @@ public override void OnStopActivity(Activity activity, object payload)
 
                 if (activityToEnrich.GetStatus().StatusCode == StatusCode.Unset)
                 {
-                    Status status = SpanHelper.ResolveSpanStatusForHttpStatusCode(response.StatusCode);
-                    activityToEnrich.SetStatus(status);
+                    activityToEnrich.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(response.StatusCode));
                 }
 
                 var routeData = context.Request.RequestContext.RouteData;

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -171,13 +171,13 @@ public override void OnStopActivity(Activity activity, object payload)
                 {
                     if (activity.GetStatus().StatusCode == StatusCode.Unset)
                     {
-                        SetStatusFromHttpStatusCode(activity, response.StatusCode);
+                        activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(response.StatusCode));
                     }
                 }
 #else
                 if (activity.GetStatus().StatusCode == StatusCode.Unset)
                 {
-                    SetStatusFromHttpStatusCode(activity, response.StatusCode);
+                    activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(response.StatusCode));
                 }
 #endif
 
@@ -304,13 +304,6 @@ private static string GetUri(HttpRequest request)
             return builder.ToString();
         }
 
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private static void SetStatusFromHttpStatusCode(Activity activity, int statusCode)
-        {
-            var status = SpanHelper.ResolveSpanStatusForHttpStatusCode(statusCode);
-            activity.SetStatus(status);
-        }
-
 #if NETSTANDARD2_1
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private static bool TryGetGrpcMethod(Activity activity, out string grpcMethod)

src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj

@@ -10,6 +10,7 @@
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\SpanAttributeConstants.cs" Link="Includes\SpanAttributeConstants.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ExceptionExtensions.cs" Link="Includes\ExceptionExtensions.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ActivityHelperExtensions.cs" Link="Includes\ActivityHelperExtensions.cs" />
+    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\StatusHelper.cs" Link="Includes\StatusHelper.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\IActivityEnumerator.cs" Link="Includes\IActivityEnumerator.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\EnumerationHelper.cs" Link="Includes\EnumerationHelper.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Instrumentation.GrpcNetClient\GrpcTagHelper.cs" Link="Includes\GrpcTagHelper.cs" />

src/OpenTelemetry.Instrumentation.GrpcNetClient/OpenTelemetry.Instrumentation.GrpcNetClient.csproj

@@ -9,6 +9,7 @@
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ExceptionExtensions.cs" Link="Includes\ExceptionExtensions.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\SemanticConventions.cs" Link="Includes\SemanticConventions.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ActivityHelperExtensions.cs" Link="Includes\ActivityHelperExtensions.cs" />
+    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\StatusHelper.cs" Link="Includes\StatusHelper.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\IActivityEnumerator.cs" Link="Includes\IActivityEnumerator.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\EnumerationHelper.cs" Link="Includes\EnumerationHelper.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\SpanAttributeConstants.cs" Link="Includes\SpanAttributeConstants.cs" />

src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+* `otel.status_description` tag will no longer be set to the http status
+  description/reason phrase for outgoing http spans.
+  ([#1579](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1579))
+
 ## 1.0.0-rc1.1
 
 Released 2020-Nov-17

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs

@@ -149,10 +149,7 @@ public override void OnStopActivity(Activity activity, object payload)
 
                     if (currentStatusCode == StatusCode.Unset)
                     {
-                        activity.SetStatus(
-                        SpanHelper
-                            .ResolveSpanStatusForHttpStatusCode((int)response.StatusCode)
-                            .WithDescription(response.ReasonPhrase));
+                        activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode((int)response.StatusCode));
                     }
 
                     try