Exporter spec fixup 2 (#1620)

* Jaeger & Zipkin status enums are now uppercase. Zipkin error tag is an empty string. * Updated CHANGELOGs. * Attempting to fix build. Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
open-telemetry · Nov 28, 2020 · 1bab62c · 1bab62c
1 parent fe9e90d
commit 1bab62c
Show file tree

Hide file tree

Showing 15 changed files with 97 additions and 81 deletions.
diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md
@@ -5,9 +5,10 @@
 * 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
+  extension) will now be set as the `UNSET`, `OK`, or `ERROR` string
   representation instead of the `0`, `1`, or `2` integer representation.
-  ([#1579](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1579))
+  ([#1579](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1579) &
+  [#1620](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1620))
 * Metrics API/SDK support is in an experimental state and is not recommended for
   production use. All metric APIs have been marked with the `Obsolete`
   attribute. See

diff --git a/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs b/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs
@@ -188,7 +188,7 @@ public bool ForEach(KeyValuePair<string, object> item)
                 switch (item.Key)
                 {
                     case SpanAttributeConstants.StatusCodeKey:
-                        this.StatusCode = StatusHelper.GetStatusCodeForStringName(item.Value as string);
+                        this.StatusCode = StatusHelper.GetStatusCodeForTagValue(item.Value as string);
                         break;
                     case SpanAttributeConstants.StatusDescriptionKey:
                         this.StatusDescription = item.Value as string;

diff --git a/src/OpenTelemetry.Api/Internal/StatusHelper.cs b/src/OpenTelemetry.Api/Internal/StatusHelper.cs
@@ -14,15 +14,20 @@
 // limitations under the License.
 // </copyright>
 
+using System;
 using System.Runtime.CompilerServices;
 using OpenTelemetry.Trace;
 
 namespace OpenTelemetry.Internal
 {
     internal static class StatusHelper
     {
+        public const string UnsetStatusCodeTagValue = "UNSET";
+        public const string OkStatusCodeTagValue = "OK";
+        public const string ErrorStatusCodeTagValue = "ERROR";
+
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public static string GetStringNameForStatusCode(StatusCode statusCode)
+        public static string GetTagValueForStatusCode(StatusCode statusCode)
         {
             return statusCode switch
             {
@@ -31,26 +36,26 @@ public static string GetStringNameForStatusCode(StatusCode statusCode)
                  * 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",
+                StatusCode.Unset => UnsetStatusCodeTagValue,
+                StatusCode.Error => ErrorStatusCodeTagValue,
+                StatusCode.Ok => OkStatusCodeTagValue,
                 _ => null,
             };
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public static StatusCode? GetStatusCodeForStringName(string statusCodeName)
+        public static StatusCode? GetStatusCodeForTagValue(string statusCodeTagValue)
         {
-            return statusCodeName switch
+            return statusCodeTagValue 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,
+                string _ when UnsetStatusCodeTagValue.Equals(statusCodeTagValue, StringComparison.OrdinalIgnoreCase) => StatusCode.Unset,
+                string _ when ErrorStatusCodeTagValue.Equals(statusCodeTagValue, StringComparison.OrdinalIgnoreCase) => StatusCode.Error,
+                string _ when OkStatusCodeTagValue.Equals(statusCodeTagValue, StringComparison.OrdinalIgnoreCase) => StatusCode.Ok,
                 _ => (StatusCode?)null,
             };
         }

diff --git a/src/OpenTelemetry.Api/README.md b/src/OpenTelemetry.Api/README.md
@@ -358,11 +358,11 @@ to be associated with `Activity`. There is no `Status` class in .NET, and hence
 Example:
 
 ```csharp
-activity?.SetTag("otel.status_code", "Error");
+activity?.SetTag("otel.status_code", "ERROR");
 activity?.SetTag("otel.status_description", "error status description");
 ```
 
-Values for the StatusCode tag must be one of the strings "Unset", "Ok", or "Error",
+Values for the StatusCode tag must be one of the strings "UNSET", "OK", or "ERROR",
 which correspond respectively to the enums `Unset`, `Ok`, and `Error` from
 [`StatusCode`](./Trace/StatusCode.cs).
 

diff --git a/src/OpenTelemetry.Api/Trace/ActivityExtensions.cs b/src/OpenTelemetry.Api/Trace/ActivityExtensions.cs
@@ -45,7 +45,7 @@ public static void SetStatus(this Activity activity, Status status)
         {
             Debug.Assert(activity != null, "Activity should not be null");
 
-            activity.SetTag(SpanAttributeConstants.StatusCodeKey, StatusHelper.GetStringNameForStatusCode(status.StatusCode));
+            activity.SetTag(SpanAttributeConstants.StatusCodeKey, StatusHelper.GetTagValueForStatusCode(status.StatusCode));
             activity.SetTag(SpanAttributeConstants.StatusDescriptionKey, status.Description);
         }
 

diff --git a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md
@@ -5,11 +5,13 @@
 * In `JaegerExporterOptions`: Exporter options now include a switch for
   Batch vs Simple exporter, and settings for batch exporting properties.
 
-* 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))
+* 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) &
+  [#1620](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1620))
 
-* Jaeger will no longer send the `otel.status_code` tag if the value is `Unset`.
-  ([#1609](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1609))
+* Jaeger will no longer send the `otel.status_code` tag if the value is `UNSET`.
+  ([#1609](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1609) &
+  [#1620](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1620))
 
 * Span Event.Name will now be populated as the `event` field on Jaeger Logs
   instead of `message`.

diff --git a/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs b/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs
@@ -259,14 +259,20 @@ private static void ProcessJaegerTag(ref TagEnumerationState state, string key,
 
                 if (key == SpanAttributeConstants.StatusCodeKey)
                 {
-                    if (jaegerTag.VStr == "Error")
+                    StatusCode? statusCode = StatusHelper.GetStatusCodeForTagValue(jaegerTag.VStr);
+                    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));
                     }
-                    else if (jaegerTag.VStr == "Unset")
+                    else if (!statusCode.HasValue || statusCode == StatusCode.Unset)
                     {
+                        // Unset Status is not sent: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk_exporters/jaeger.md#status
                         return;
                     }
+
+                    // Normalize status since it is user-driven.
+                    jaegerTag = new JaegerTag(key, JaegerTagType.STRING, vStr: StatusHelper.GetTagValueForStatusCode(statusCode.Value));
                 }
             }
             else if (jaegerTag.VLong.HasValue)

diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ActivityExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ActivityExtensions.cs
@@ -239,7 +239,7 @@ internal static OtlpCommon.KeyValue ToOtlpAttribute(this KeyValuePair<string, ob
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private static OtlpTrace.Status ToOtlpStatus(ref TagEnumerationState otlpTags)
         {
-            var status = StatusHelper.GetStatusCodeForStringName(otlpTags.StatusCode);
+            var status = StatusHelper.GetStatusCodeForTagValue(otlpTags.StatusCode);
 
             if (!status.HasValue)
             {

diff --git a/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md b/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md
@@ -2,11 +2,13 @@
 
 ## 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))
+* 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 no longer send the `otel.status_code` tag if the value is `Unset`.
-  ([#1609](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1609))
+* 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) &
+  [#1620](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1620))
 
 * Zipkin bool tag values will now be sent as `true`/`false` instead of
   `True`/`False`.

diff --git a/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs b/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs
@@ -177,15 +177,20 @@ public bool ForEach(KeyValuePair<string, object> activityTag)
 
                     if (key == SpanAttributeConstants.StatusCodeKey)
                     {
-                        if (strVal == "Error")
+                        StatusCode? statusCode = StatusHelper.GetStatusCodeForTagValue(strVal);
+                        if (statusCode == StatusCode.Error)
                         {
-                            PooledList<KeyValuePair<string, object>>.Add(ref this.Tags, new KeyValuePair<string, object>("error", "true"));
+                            // 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 (strVal == "Unset")
+                        else if (!statusCode.HasValue || statusCode == 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));
                     }
                 }
                 else if (activityTag.Value is int intVal && activityTag.Key == SemanticConventions.AttributeNetPeerPort)

diff --git a/test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs b/test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs
@@ -436,39 +436,35 @@ public void JaegerActivityConverterTest_NullTagValueTest()
         }
 
         [Theory]
-        [InlineData(StatusCode.Unset, false)]
-        [InlineData(StatusCode.Ok, false)]
-        [InlineData(StatusCode.Error, true)]
-        public void JaegerActivityConverterTest_Status_ErrorFlagTest(StatusCode statusCode, bool hasErrorFlag)
+        [InlineData(StatusCode.Unset, "unset")]
+        [InlineData(StatusCode.Ok, "Ok")]
+        [InlineData(StatusCode.Error, "ERROR")]
+        [InlineData(StatusCode.Unset, "iNvAlId")]
+        public void JaegerActivityConverterTest_Status_ErrorFlagTest(StatusCode expectedStatusCode, string statusCodeTagValue)
         {
-            var status = statusCode switch
-            {
-                StatusCode.Unset => Status.Unset,
-                StatusCode.Ok => Status.Ok,
-                StatusCode.Error => Status.Error,
-                _ => throw new InvalidOperationException(),
-            };
-
             // Arrange
-            var activity = CreateTestActivity(status: status);
+            var activity = CreateTestActivity();
+            activity.SetTag(SpanAttributeConstants.StatusCodeKey, statusCodeTagValue);
 
             // Act
             var jaegerSpan = activity.ToJaegerSpan();
 
             // Assert
 
-            if (statusCode == StatusCode.Unset)
+            Assert.Equal(expectedStatusCode, activity.GetStatus().StatusCode);
+
+            if (expectedStatusCode == StatusCode.Unset)
             {
                 Assert.DoesNotContain(jaegerSpan.Tags, t => t.Key == SpanAttributeConstants.StatusCodeKey);
             }
             else
             {
                 Assert.Equal(
-                    StatusHelper.GetStringNameForStatusCode(statusCode),
+                    StatusHelper.GetTagValueForStatusCode(expectedStatusCode),
                     jaegerSpan.Tags.FirstOrDefault(t => t.Key == SpanAttributeConstants.StatusCodeKey).VStr);
             }
 
-            if (hasErrorFlag)
+            if (expectedStatusCode == StatusCode.Error)
             {
                 Assert.Contains(jaegerSpan.Tags, t => t.Key == "error" && t.VType == JaegerTagType.BOOL && (t.VBool ?? false));
             }

diff --git a/test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityConversionTest.cs b/test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityConversionTest.cs
@@ -89,41 +89,37 @@ public void ToZipkinSpan_NoEvents()
         }
 
         [Theory]
-        [InlineData(StatusCode.Unset, false)]
-        [InlineData(StatusCode.Ok, false)]
-        [InlineData(StatusCode.Error, true)]
-        public void ToZipkinSpan_Status_ErrorFlagTest(StatusCode statusCode, bool hasErrorFlag)
+        [InlineData(StatusCode.Unset, "unset")]
+        [InlineData(StatusCode.Ok, "Ok")]
+        [InlineData(StatusCode.Error, "ERROR")]
+        [InlineData(StatusCode.Unset, "iNvAlId")]
+        public void ToZipkinSpan_Status_ErrorFlagTest(StatusCode expectedStatusCode, string statusCodeTagValue)
         {
-            var status = statusCode switch
-            {
-                StatusCode.Unset => Status.Unset,
-                StatusCode.Ok => Status.Ok,
-                StatusCode.Error => Status.Error,
-                _ => throw new InvalidOperationException(),
-            };
-
             // Arrange
-            var activity = ZipkinExporterTests.CreateTestActivity(status: status);
+            var activity = ZipkinExporterTests.CreateTestActivity();
+            activity.SetTag(SpanAttributeConstants.StatusCodeKey, statusCodeTagValue);
 
             // Act
             var zipkinSpan = activity.ToZipkinSpan(DefaultZipkinEndpoint);
 
             // Assert
 
-            if (statusCode == StatusCode.Unset)
+            Assert.Equal(expectedStatusCode, activity.GetStatus().StatusCode);
+
+            if (expectedStatusCode == StatusCode.Unset)
             {
                 Assert.DoesNotContain(zipkinSpan.Tags, t => t.Key == SpanAttributeConstants.StatusCodeKey);
             }
             else
             {
                 Assert.Equal(
-                    StatusHelper.GetStringNameForStatusCode(statusCode),
+                    StatusHelper.GetTagValueForStatusCode(expectedStatusCode),
                     zipkinSpan.Tags.FirstOrDefault(t => t.Key == SpanAttributeConstants.StatusCodeKey).Value);
             }
 
-            if (hasErrorFlag)
+            if (expectedStatusCode == StatusCode.Error)
             {
-                Assert.Contains(zipkinSpan.Tags, t => t.Key == "error" && (string)t.Value == "true");
+                Assert.Contains(zipkinSpan.Tags, t => t.Key == "error" && (string)t.Value == string.Empty);
             }
             else
             {

diff --git a/test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs b/test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs
@@ -137,8 +137,18 @@ public void SuppresssesInstrumentation()
         [InlineData(false, false, false)]
         [InlineData(false, true, false)]
         [InlineData(false, false, true)]
-        public void IntegrationTest(bool useShortTraceIds, bool useTestResource, bool isRootSpan)
+        [InlineData(false, false, false, StatusCode.Ok)]
+        [InlineData(false, false, false, StatusCode.Error)]
+        public void IntegrationTest(bool useShortTraceIds, bool useTestResource, bool isRootSpan, StatusCode statusCode = StatusCode.Unset)
         {
+            var status = statusCode switch
+            {
+                StatusCode.Unset => Status.Unset,
+                StatusCode.Ok => Status.Ok,
+                StatusCode.Error => Status.Error,
+                _ => throw new InvalidOperationException(),
+            };
+
             Guid requestId = Guid.NewGuid();
 
             ZipkinExporter exporter = new ZipkinExporter(
@@ -150,7 +160,7 @@ public void IntegrationTest(bool useShortTraceIds, bool useTestResource, bool is
 
             var serviceName = ZipkinExporterOptions.DefaultServiceName;
             var resoureTags = string.Empty;
-            var activity = CreateTestActivity(isRootSpan: isRootSpan);
+            var activity = CreateTestActivity(isRootSpan: isRootSpan, status: status);
             if (useTestResource)
             {
                 serviceName = "MyService";
@@ -192,8 +202,15 @@ public void IntegrationTest(bool useShortTraceIds, bool useTestResource, bool is
 
             var traceId = useShortTraceIds ? TraceId.Substring(TraceId.Length - 16, 16) : TraceId;
 
+            var statusTag = statusCode switch
+            {
+                StatusCode.Ok => $@"""{SpanAttributeConstants.StatusCodeKey}"":""OK"",",
+                StatusCode.Error => $@"""error"":"""",""{SpanAttributeConstants.StatusCodeKey}"":""ERROR"",",
+                _ => string.Empty,
+            };
+
             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/"",""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/""}}}}]",
                 Responses[requestId]);
         }
 

diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs
@@ -96,17 +96,10 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut
             Assert.Equal(ActivityKind.Client, activity.Kind);
             Assert.Equal(tc.SpanName, activity.DisplayName);
 
-            var d = new Dictionary<string, string>()
-            {
-                { "Ok", "OK" },
-                { "Error", "ERROR" },
-                { "Unset", "UNSET" },
-            };
-
             // Assert.Equal(tc.SpanStatus, d[span.Status.CanonicalCode]);
             Assert.Equal(
                     tc.SpanStatus,
-                    d[activity.GetTagValue(SpanAttributeConstants.StatusCodeKey) as string]);
+                    activity.GetTagValue(SpanAttributeConstants.StatusCodeKey) as string);
 
             if (tc.SpanStatusHasDescription.HasValue)
             {