Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Status & error flag updates to match spec #1655

Merged
merged 2 commits into from
Dec 14, 2020
Merged

Status & error flag updates to match spec #1655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d60af1
Status & error flag updates to match spec.
CodeBlanch Dec 11, 2020
15f2bfc
CHANGELOG updates.
CodeBlanch Dec 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Api/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
9 changes: 8 additions & 1 deletion src/OpenTelemetry.Api/Trace/Status.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
9 changes: 8 additions & 1 deletion src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
{
Expand All @@ -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)
{
Expand Down
9 changes: 6 additions & 3 deletions src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) &
Expand Down
38 changes: 30 additions & 8 deletions src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
43 changes: 36 additions & 7 deletions test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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(
Expand Down Expand Up @@ -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);
Expand All @@ -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]);
}

Expand Down
17 changes: 17 additions & 0 deletions test/OpenTelemetry.Tests/Trace/ActivityExtensionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down