diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md index a2e20890cb7..868c94673d8 100644 --- a/src/OpenTelemetry.Api/CHANGELOG.md +++ b/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 diff --git a/src/OpenTelemetry.Api/Trace/Status.cs b/src/OpenTelemetry.Api/Trace/Status.cs index 7ba14aac142..ca9ca625104 100644 --- a/src/OpenTelemetry.Api/Trace/Status.cs +++ b/src/OpenTelemetry.Api/Trace/Status.cs @@ -69,11 +69,18 @@ internal Status(StatusCode statusCode, string description = null) /// /// Returns a new instance of a status with the description populated. /// + /// + /// Note: Status Description is only valid for Status and will be ignored for all other + /// values. See the Status + /// API for details. + /// /// Description of the status. /// New instance of the status class with the description populated. public Status WithDescription(string description) { - if (this.Description == description) + if (this.StatusCode != StatusCode.Error || this.Description == description) { return this; } diff --git a/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs b/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs index 5a48163439d..9cf4b187034 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs +++ b/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.Add(ref state.Tags, new JaegerTag("error", JaegerTagType.BOOL, vBool: true)); + PooledList.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) { diff --git a/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md b/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md index 56d10464217..0bad17af44c 100644 --- a/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md +++ b/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) & diff --git a/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs b/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs index 6f183823f9c..aae1dca2685 100644 --- a/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs +++ b/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>.Add( + ref tagState.Tags, + new KeyValuePair( + ZipkinErrorFlagTagName, + tagState.StatusDescription ?? string.Empty)); + } + EventEnumerationState eventState = default; activity.EnumerateEvents(ref eventState); @@ -162,6 +173,10 @@ internal struct TagEnumerationState : IActivityEnumerator activityTag) { if (activityTag.Value == null) @@ -177,20 +192,27 @@ public bool ForEach(KeyValuePair 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>.Add(ref this.Tags, new KeyValuePair("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(key, StatusHelper.GetTagValueForStatusCode(statusCode.Value)); + activityTag = new KeyValuePair(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) diff --git a/test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs b/test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs index defdb0465dc..b691a66b5ec 100644 --- a/test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs +++ b/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); } } diff --git a/test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs b/test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs index 4446f5fc001..10c2e248e04 100644 --- a/test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs +++ b/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(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]); } diff --git a/test/OpenTelemetry.Tests/Trace/ActivityExtensionsTest.cs b/test/OpenTelemetry.Tests/Trace/ActivityExtensionsTest.cs index 4df4a6a8b82..35e55ec5be8 100644 --- a/test/OpenTelemetry.Tests/Trace/ActivityExtensionsTest.cs +++ b/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() {