Status & error flag updates to match spec (#1655)

* Status & error flag updates to match spec.

* CHANGELOG updates.

src/OpenTelemetry.Api/CHANGELOG.md

@@ -15,6 +15,9 @@
   [#1501](https://github.com/open-telemetry/opentelemetry-dotnet/issues/1501)
   for more information.
   ([#1611](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1611))
+* `Status.WithDescription` will now ignore the provided description if the
+  `Status.StatusCode` is anything other than `ERROR`.
+  ([#1655](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1655))
 
 ## 1.0.0-rc1.1
 

src/OpenTelemetry.Api/Trace/Status.cs

@@ -69,11 +69,18 @@ internal Status(StatusCode statusCode, string description = null)
         /// <summary>
         /// Returns a new instance of a status with the description populated.
         /// </summary>
+        /// <remarks>
+        /// Note: Status Description is only valid for <see
+        /// cref="StatusCode.Error"/> Status and will be ignored for all other
+        /// <see cref="Trace.StatusCode"/> values. See the <a
+        /// href="https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-status">Status
+        /// API</a> for details.
+        /// </remarks>
         /// <param name="description">Description of the status.</param>
         /// <returns>New instance of the status class with the description populated.</returns>
         public Status WithDescription(string description)
         {
-            if (this.Description == description)
+            if (this.StatusCode != StatusCode.Error || this.Description == description)
             {
                 return this;
             }

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

@@ -25,6 +25,8 @@ namespace OpenTelemetry.Exporter.Jaeger.Implementation
 {
     internal static class JaegerActivityExtensions
     {
+        internal const string JaegerErrorFlagTagName = "error";
+
         private const int DaysPerYear = 365;
 
         // Number of days in 4 years
@@ -263,7 +265,7 @@ private static void ProcessJaegerTag(ref TagEnumerationState state, string key,
                     if (statusCode == StatusCode.Error)
                     {
                         // Error flag: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk_exporters/jaeger.md#error-flag
-                        PooledList<JaegerTag>.Add(ref state.Tags, new JaegerTag("error", JaegerTagType.BOOL, vBool: true));
+                        PooledList<JaegerTag>.Add(ref state.Tags, new JaegerTag(JaegerErrorFlagTagName, JaegerTagType.BOOL, vBool: true));
                     }
                     else if (!statusCode.HasValue || statusCode == StatusCode.Unset)
                     {
@@ -274,6 +276,11 @@ private static void ProcessJaegerTag(ref TagEnumerationState state, string key,
                     // Normalize status since it is user-driven.
                     jaegerTag = new JaegerTag(key, JaegerTagType.STRING, vStr: StatusHelper.GetTagValueForStatusCode(statusCode.Value));
                 }
+                else if (key == JaegerErrorFlagTagName)
+                {
+                    // Ignore `error` tag if it exists, it will be added based on StatusCode + StatusDescription.
+                    return;
+                }
             }
             else if (jaegerTag.VLong.HasValue)
             {

src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md

@@ -5,9 +5,12 @@
 * Changed `ZipkinExporter` class and constructor from internal to public.
   ([#1612](https://github.com/open-telemetry/opentelemetry-dotnet/issues/1612))
 
-* 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) &
-  [#1620](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1620))
+* Zipkin will now set the `error` tag to the `Status.Description` value or an
+  empty string when `Status.StatusCode` (`otel.status_code` tag) is set to
+  `ERROR`.
+  ([#1579](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1579),
+  [#1620](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1620), &
+  [#1655](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1655))
 
 * Zipkin will no longer send the `otel.status_code` tag if the value is `UNSET`.
   ([#1609](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1609) &

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

@@ -25,6 +25,7 @@ namespace OpenTelemetry.Exporter.Zipkin.Implementation
 {
     internal static class ZipkinActivityConversionExtensions
     {
+        internal const string ZipkinErrorFlagTagName = "error";
         private const long TicksPerMicrosecond = TimeSpan.TicksPerMillisecond / 1000;
         private const long UnixEpochTicks = 621355968000000000L; // = DateTimeOffset.FromUnixTimeMilliseconds(0).Ticks
         private const long UnixEpochMicroseconds = UnixEpochTicks / TicksPerMicrosecond;
@@ -79,6 +80,16 @@ internal static ZipkinSpan ToZipkinSpan(this Activity activity, ZipkinEndpoint l
                 }
             }
 
+            if (tagState.StatusCode == StatusCode.Error)
+            {
+                // Error flag rule from https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk_exporters/zipkin.md#status
+                PooledList<KeyValuePair<string, object>>.Add(
+                    ref tagState.Tags,
+                    new KeyValuePair<string, object>(
+                        ZipkinErrorFlagTagName,
+                        tagState.StatusDescription ?? string.Empty));
+            }
+
             EventEnumerationState eventState = default;
             activity.EnumerateEvents(ref eventState);
 
@@ -162,6 +173,10 @@ internal struct TagEnumerationState : IActivityEnumerator<KeyValuePair<string, o
 
             public long Port { get; set; }
 
+            public StatusCode? StatusCode { get; set; }
+
+            public string StatusDescription { get; set; }
+
             public bool ForEach(KeyValuePair<string, object> activityTag)
             {
                 if (activityTag.Value == null)
@@ -177,20 +192,27 @@ public bool ForEach(KeyValuePair<string, object> activityTag)
 
                     if (key == SpanAttributeConstants.StatusCodeKey)
                     {
-                        StatusCode? statusCode = StatusHelper.GetStatusCodeForTagValue(strVal);
-                        if (statusCode == StatusCode.Error)
-                        {
-                            // Error flag: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk_exporters/zipkin.md#error-flag
-                            PooledList<KeyValuePair<string, object>>.Add(ref this.Tags, new KeyValuePair<string, object>("error", string.Empty));
-                        }
-                        else if (!statusCode.HasValue || statusCode == StatusCode.Unset)
+                        this.StatusCode = StatusHelper.GetStatusCodeForTagValue(strVal);
+
+                        if (!this.StatusCode.HasValue || this.StatusCode == Trace.StatusCode.Unset)
                         {
                             // Unset Status is not sent: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk_exporters/zipkin.md#status
                             return true;
                         }
 
                         // Normalize status since it is user-driven.
-                        activityTag = new KeyValuePair<string, object>(key, StatusHelper.GetTagValueForStatusCode(statusCode.Value));
+                        activityTag = new KeyValuePair<string, object>(key, StatusHelper.GetTagValueForStatusCode(this.StatusCode.Value));
+                    }
+                    else if (key == SpanAttributeConstants.StatusDescriptionKey)
+                    {
+                        // Description is sent as `error` but only if StatusCode is Error. See: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk_exporters/zipkin.md#status
+                        this.StatusDescription = strVal;
+                        return true;
+                    }
+                    else if (key == ZipkinErrorFlagTagName)
+                    {
+                        // Ignore `error` tag if it exists, it will be added based on StatusCode + StatusDescription.
+                        return true;
                     }
                 }
                 else if (activityTag.Value is int intVal && activityTag.Key == SemanticConventions.AttributeNetPeerPort)

test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs

@@ -466,11 +466,11 @@ public void JaegerActivityConverterTest_Status_ErrorFlagTest(StatusCode expected
 
             if (expectedStatusCode == StatusCode.Error)
             {
-                Assert.Contains(jaegerSpan.Tags, t => t.Key == "error" && t.VType == JaegerTagType.BOOL && (t.VBool ?? false));
+                Assert.Contains(jaegerSpan.Tags, t => t.Key == JaegerActivityExtensions.JaegerErrorFlagTagName && t.VType == JaegerTagType.BOOL && (t.VBool ?? false));
             }
             else
             {
-                Assert.DoesNotContain(jaegerSpan.Tags, t => t.Key == "error");
+                Assert.DoesNotContain(jaegerSpan.Tags, t => t.Key == JaegerActivityExtensions.JaegerErrorFlagTagName);
             }
         }
 

test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs

@@ -138,8 +138,16 @@ public void SuppresssesInstrumentation()
         [InlineData(false, true, false)]
         [InlineData(false, false, true)]
         [InlineData(false, false, false, StatusCode.Ok)]
+        [InlineData(false, false, false, StatusCode.Ok, null, true)]
         [InlineData(false, false, false, StatusCode.Error)]
-        public void IntegrationTest(bool useShortTraceIds, bool useTestResource, bool isRootSpan, StatusCode statusCode = StatusCode.Unset)
+        [InlineData(false, false, false, StatusCode.Error, "Error description")]
+        public void IntegrationTest(
+            bool useShortTraceIds,
+            bool useTestResource,
+            bool isRootSpan,
+            StatusCode statusCode = StatusCode.Unset,
+            string statusDescription = null,
+            bool addErrorTag = false)
         {
             var status = statusCode switch
             {
@@ -149,6 +157,11 @@ public void IntegrationTest(bool useShortTraceIds, bool useTestResource, bool is
                 _ => throw new InvalidOperationException(),
             };
 
+            if (!string.IsNullOrEmpty(statusDescription))
+            {
+                status = status.WithDescription(statusDescription);
+            }
+
             Guid requestId = Guid.NewGuid();
 
             ZipkinExporter exporter = new ZipkinExporter(
@@ -178,6 +191,11 @@ public void IntegrationTest(bool useShortTraceIds, bool useTestResource, bool is
                 exporter.SetLocalEndpointFromResource(Resource.Empty);
             }
 
+            if (addErrorTag)
+            {
+                activity.SetTag(ZipkinActivityConversionExtensions.ZipkinErrorFlagTagName, "This should be removed.");
+            }
+
             var processor = new SimpleExportProcessor<Activity>(exporter);
 
             processor.OnEnd(activity);
@@ -202,15 +220,26 @@ public void IntegrationTest(bool useShortTraceIds, bool useTestResource, bool is
 
             var traceId = useShortTraceIds ? TraceId.Substring(TraceId.Length - 16, 16) : TraceId;
 
-            var statusTag = statusCode switch
+            string statusTag;
+            string errorTag = string.Empty;
+            switch (statusCode)
             {
-                StatusCode.Ok => $@"""{SpanAttributeConstants.StatusCodeKey}"":""OK"",",
-                StatusCode.Error => $@"""error"":"""",""{SpanAttributeConstants.StatusCodeKey}"":""ERROR"",",
-                _ => string.Empty,
-            };
+                case StatusCode.Ok:
+                    statusTag = $@"""{SpanAttributeConstants.StatusCodeKey}"":""OK"",";
+                    break;
+                case StatusCode.Unset:
+                    statusTag = string.Empty;
+                    break;
+                case StatusCode.Error:
+                    statusTag = $@"""{SpanAttributeConstants.StatusCodeKey}"":""ERROR"",";
+                    errorTag = $@",""{ZipkinActivityConversionExtensions.ZipkinErrorFlagTagName}"":""{statusDescription}""";
+                    break;
+                default:
+                    throw new NotSupportedException();
+            }
 
             Assert.Equal(
-                $@"[{{""traceId"":""{traceId}"",""name"":""Name"",{parentId}""id"":""{ZipkinActivityConversionExtensions.EncodeSpanId(context.SpanId)}"",""kind"":""CLIENT"",""timestamp"":{timestamp},""duration"":60000000,""localEndpoint"":{{""serviceName"":""{serviceName}""{ipInformation}}},""remoteEndpoint"":{{""serviceName"":""http://localhost:44312/""}},""annotations"":[{{""timestamp"":{eventTimestamp},""value"":""Event1""}},{{""timestamp"":{eventTimestamp},""value"":""Event2""}}],""tags"":{{{resoureTags}""stringKey"":""value"",""longKey"":""1"",""longKey2"":""1"",""doubleKey"":""1"",""doubleKey2"":""1"",""longArrayKey"":""1,2"",""boolKey"":""true"",""boolArrayKey"":""true,false"",""http.host"":""http://localhost:44312/"",{statusTag}""otel.library.name"":""CreateTestActivity"",""peer.service"":""http://localhost:44312/""}}}}]",
+                $@"[{{""traceId"":""{traceId}"",""name"":""Name"",{parentId}""id"":""{ZipkinActivityConversionExtensions.EncodeSpanId(context.SpanId)}"",""kind"":""CLIENT"",""timestamp"":{timestamp},""duration"":60000000,""localEndpoint"":{{""serviceName"":""{serviceName}""{ipInformation}}},""remoteEndpoint"":{{""serviceName"":""http://localhost:44312/""}},""annotations"":[{{""timestamp"":{eventTimestamp},""value"":""Event1""}},{{""timestamp"":{eventTimestamp},""value"":""Event2""}}],""tags"":{{{resoureTags}""stringKey"":""value"",""longKey"":""1"",""longKey2"":""1"",""doubleKey"":""1"",""doubleKey2"":""1"",""longArrayKey"":""1,2"",""boolKey"":""true"",""boolArrayKey"":""true,false"",""http.host"":""http://localhost:44312/"",{statusTag}""otel.library.name"":""CreateTestActivity"",""peer.service"":""http://localhost:44312/""{errorTag}}}}}]",
                 Responses[requestId]);
         }
 

test/OpenTelemetry.Tests/Trace/ActivityExtensionsTest.cs

@@ -77,6 +77,23 @@ public void SetStatusWithDescriptionTwice()
             Assert.Null(status.Description);
         }
 
+        [Fact]
+        public void SetStatusWithIgnoredDescription()
+        {
+            using var openTelemetrySdk = Sdk.CreateTracerProviderBuilder()
+                .AddSource(ActivitySourceName)
+                .Build();
+
+            using var source = new ActivitySource(ActivitySourceName);
+            using var activity = source.StartActivity(ActivityName);
+            activity.SetStatus(Status.Ok.WithDescription("This should be ignored."));
+            activity?.Stop();
+
+            var status = activity.GetStatus();
+            Assert.Equal(StatusCode.Ok, status.StatusCode);
+            Assert.Null(status.Description);
+        }
+
         [Fact]
         public void SetCancelledStatus()
         {