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

Add support for Activity Status and StatusDescription in Zipkin Exporter #2572

Closed
wants to merge 9 commits into from
Closed

Add support for Activity Status and StatusDescription in Zipkin Exporter #2572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4fe845c
draft
Nov 8, 2021
f45aaed
Status and StatusDescription precedence
Nov 9, 2021
86c8800
Merge branch 'main' into vibankwa/Add-Status-AndStatusDescription-Sup…
Nov 9, 2021
8ea3903
fix precendence
Nov 9, 2021
52c5bf6
fix status value
Nov 9, 2021
04f267c
changelog
Nov 9, 2021
78402a0
Merge branch 'main' into vibankwa/Add-Status-AndStatusDescription-Sup…
vishweshbankwar Nov 9, 2021
1eec129
integration test
Nov 9, 2021
82a3f1d
Trigger Build
Nov 9, 2021
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 @@ -2,6 +2,9 @@

## Unreleased

* Added support for `Status` and `StatusDescription` properties on Activity
([#2572](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2572))

## 1.2.0-beta1

Released 2021-Oct-08
Expand Down
18 changes: 18 additions & 0 deletions src/OpenTelemetry.Api/Internal/StatusHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// </copyright>

using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using OpenTelemetry.Trace;

Expand Down Expand Up @@ -43,6 +44,23 @@ public static string GetTagValueForStatusCode(StatusCode statusCode)
};
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static string GetTagValueForActivityStatusCode(ActivityStatusCode activityStatusCode)
{
return activityStatusCode switch
{
/*
* Note: Order here does matter for perf. Unset is
* first because assumption is most spans will be
* Unset, then Error, then Ok.
*/
ActivityStatusCode.Unset => UnsetStatusCodeTagValue,
ActivityStatusCode.Error => ErrorStatusCodeTagValue,
ActivityStatusCode.Ok => OkStatusCodeTagValue,
_ => null,
};
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static StatusCode? GetStatusCodeForTagValue(string statusCodeTagValue)
{
Expand Down
54 changes: 40 additions & 14 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 @@ -72,14 +72,44 @@ internal static ZipkinSpan ToZipkinSpan(this Activity activity, ZipkinEndpoint l
}
}

if (tagState.StatusCode == StatusCode.Error)
// Starting version 6.0.0 in System.Diagnostic.DiagnosticSource
// Status and StatusDescription can be set using activity.SetStatus(ActivityStatusCode, string)
// Set otel.status_code and error to Status and StatusDescription respectively (If available)
if (activity.Status == ActivityStatusCode.Ok || activity.Status == ActivityStatusCode.Error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this break existing users relying on the extensions which set tags for status? I haven't had a chance to look at this fully yet, but I think we need a migration strategy. It might be possible to do that upstream so we don't have to modify all the existing exporters.

Copy link
Member Author

@vishweshbankwar vishweshbankwar Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it won't - we will still look up those tags in case Status and StatusDescription is not set. Could you please elaborate on "upstream change"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it thanks.

The thought I have is, this is a lot of code and sort of tribal knowledge to expect every exporter to understand and do correctly. Ideally, we smooth that out so that they don't need to worry about it.

Some unknowns...

  • What do we do with this:

    opentelemetry-dotnet/src/OpenTelemetry.Api/Trace/ActivityExtensions.cs

    Line 44 in 0ed4ed6

    public static void SetStatus(this Activity activity, Status status)

    Should it be updated to set activity.Status? I feel like yes.

  • What about people manually adding otel.status_code & otel.status_description? Right now we check for those and set error flags in zipkin and jaeger because we didn't have Activity.Status but if we change the extension method, should we continue to do that?

One random idea as an upstream example we could set otel.status_code and otel.status_descriptions tags from activity.Status/Description (or do the reverse) in a processor as a kind of migration helper people could decide to use or not use. Not saying it is the best option 😄

Basically I think we need to have a strategy for migrating to the new API. For Status and probably the .NET propagation stuff in .NET 6.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can mark ActivityExtension method SetStatus as [Obsolete] with a message saying "Please use the native activity.SetStatus instead", right?

Copy link
Member Author

@vishweshbankwar vishweshbankwar Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it thanks.

The thought I have is, this is a lot of code and sort of tribal knowledge to expect every exporter to understand and do correctly. Ideally, we smooth that out so that they don't need to worry about it.

Some unknowns...

[VB] We cannot change the behavior of SetStatus() as it will be a breaking change then. The idea here is to keep extensions as is so anyone using it or continues to use it will not be affected. We will mark SetStatus from extensions obsolete and remove it in next major version change 2.x. I have noted some of these points in issue here - #2569.

  • What about people manually adding otel.status_code & otel.status_description? Right now we check for those and set error flags in zipkin and jaeger because we didn't have Activity.Status but if we change the extension method, should we continue to do that?

[VB] This should continue to work with this change. Anyone migrating to activity.SetStatus(ActivityStatusCode, desc) will also get correct status exported. Agree that exporters have to do extra work till we retire SetStatus(StatusCode)

One random idea as an upstream example we could set otel.status_code and otel.status_descriptions tags from activity.Status/Description (or do the reverse) in a processor as a kind of migration helper people could decide to use or not use. Not saying it is the best option 😄

[VB] In future we would want to move away from setting tags and only use Status and StatusDescription properties on Activity. Leave it to exporters how the status is exported.

Basically I think we need to have a strategy for migrating to the new API. For Status and probably the .NET propagation stuff in .NET 6.
@CodeBlanch - Please see my responses above inline [VB]. Can continue more discussion here #2569
cc: @cijothomas

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @vishweshbankwar hadn't seen the issue! If we Obsolete the extension, the idea is in the future we would remove it and then also the logic looking for the tags in the exporters?

[VB] We cannot change the behavior of SetStatus() as it will be a breaking change then. The idea here is to keep extensions as is so anyone using it or continues to use it will not be affected.

I hear what you are saying, but I don't know if I 100% agree. If we changed it to chain to activity.Status/Description the API would be the same, only the implementation would change. And it is our prerogative to change our implementation details.

Anyway, I'm OK with the plan. Would still prefer to find a way to do this upstream. Normalize everything onto the Activity API so exporter authors only need to deal with that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... but I think we need a migration strategy. It might be possible to do that upstream so we don't have to modify all the existing exporters.

+1, it'll be great if we can clarify the migration story/strategy (in the issue or PR description).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would still prefer to find a way to do this upstream -- this'd be really nice!

Were you thinking something like
The SDK "checks status inside Tags, and based on that updates the Activity.Status, if not already set", so exporters can be modified to simply look Activity.Status?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cijothomas

Were you thinking something like
The SDK "checks status inside Tags, and based on that updates the Activity.Status, if not already set", so exporters can be modified to simply look Activity.Status?

That was the first idea I had, ya. Inside the SDK somewhere, maybe a migration processor people could toggle as needed. What if we had a compatibility level switch somewhere?

Copy link
Member Author

@vishweshbankwar vishweshbankwar Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cijothomas @CodeBlanch - Could you please help me understand the following:

How do we remove looking up tags for status in exporters - Wouldn't this be a breaking change in exporters?

If as a user I am using SetTag method to set status, Wouldn't I expect it to continue working without adding processor or switch?

Also, what is the end goal?
Do we want users to move away completely from using tags/SetStatus(StatusCode)?

{
// Error flag rule from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/zipkin.md#status
PooledList<KeyValuePair<string, object>>.Add(
ref tagState.Tags,
new KeyValuePair<string, object>(
ZipkinErrorFlagTagName,
tagState.StatusDescription ?? string.Empty));
ref tagState.Tags,
new KeyValuePair<string, object>(
SpanAttributeConstants.StatusCodeKey,
StatusHelper.GetTagValueForActivityStatusCode(activity.Status)));

if (activity.Status == ActivityStatusCode.Error)
{
// Error flag rule from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/zipkin.md#status
PooledList<KeyValuePair<string, object>>.Add(
ref tagState.Tags,
new KeyValuePair<string, object>(
ZipkinErrorFlagTagName,
activity.StatusDescription ?? string.Empty));
}
}
else if (tagState.StatusCode.HasValue && tagState.StatusCode != StatusCode.Unset)
{
PooledList<KeyValuePair<string, object>>.Add(
ref tagState.Tags,
new KeyValuePair<string, object>(
SpanAttributeConstants.StatusCodeKey,
StatusHelper.GetTagValueForStatusCode(tagState.StatusCode.Value)));

if (tagState.StatusCode == StatusCode.Error)
{
// Error flag rule from https://github.com/open-telemetry/opentelemetry-specification/blob/main/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;
Expand Down Expand Up @@ -186,14 +216,10 @@ public bool ForEach(KeyValuePair<string, object> activityTag)
{
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/main/specification/trace/sdk_exporters/zipkin.md#status
return true;
}

// Normalize status since it is user-driven.
activityTag = new KeyValuePair<string, object>(key, StatusHelper.GetTagValueForStatusCode(this.StatusCode.Value));
// Return without adding it to output tag.
// It will be added later after checking Activity.Status property
// If Activity.Status is set then it will be preferred over otel.status_code tag on Activity.
return true;
}
else if (key == SpanAttributeConstants.StatusDescriptionKey)
{
Expand Down
66 changes: 66 additions & 0 deletions test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityConversionTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// limitations under the License.
// </copyright>

using System.Diagnostics;
using System.Linq;
using OpenTelemetry.Exporter.Zipkin.Tests;
using OpenTelemetry.Internal;
Expand Down Expand Up @@ -125,5 +126,70 @@ public void ToZipkinSpan_Status_ErrorFlagTest(StatusCode expectedStatusCode, str
Assert.DoesNotContain(zipkinSpan.Tags, t => t.Key == "error");
}
}

[Theory]
[InlineData(ActivityStatusCode.Unset)]
[InlineData(ActivityStatusCode.Ok)]
[InlineData(ActivityStatusCode.Error)]
public void ToZipkinSpan_Activity_Status_And_StatusDescription_is_Set(ActivityStatusCode expectedStatusCode)
{
// Arrange
var activity = ZipkinExporterTests.CreateTestActivity();

activity.SetStatus(expectedStatusCode);

// Act
var zipkinSpan = activity.ToZipkinSpan(DefaultZipkinEndpoint);

// Assert
if (expectedStatusCode == ActivityStatusCode.Unset)
{
Assert.DoesNotContain(zipkinSpan.Tags, t => t.Key == SpanAttributeConstants.StatusCodeKey);
}
else
{
Assert.Equal(
StatusHelper.GetTagValueForActivityStatusCode(expectedStatusCode),
zipkinSpan.Tags.FirstOrDefault(t => t.Key == SpanAttributeConstants.StatusCodeKey).Value);
}

if (expectedStatusCode == ActivityStatusCode.Error)
{
Assert.Contains(zipkinSpan.Tags, t => t.Key == "error" && (string)t.Value == string.Empty);
}
else
{
Assert.DoesNotContain(zipkinSpan.Tags, t => t.Key == "error");
}
}

[Theory]
[InlineData(ActivityStatusCode.Ok, "ERROR")]
[InlineData(ActivityStatusCode.Error, "OK")]
public void ActivityStatus_Takes_precedence_Over_Status_Tags(ActivityStatusCode activityStatus, string statusCodeTagValue)
{
// Arrange
var activity = ZipkinExporterTests.CreateTestActivity();

activity.SetStatus(activityStatus);
activity.SetTag(SpanAttributeConstants.StatusCodeKey, statusCodeTagValue);

// Act
var zipkinSpan = activity.ToZipkinSpan(DefaultZipkinEndpoint);

// Assert
Assert.Equal(
StatusHelper.GetTagValueForActivityStatusCode(activityStatus),
zipkinSpan.Tags.FirstOrDefault(t => t.Key == SpanAttributeConstants.StatusCodeKey).Value);

if (activityStatus == ActivityStatusCode.Error)
{
Assert.Contains(zipkinSpan.Tags, t => t.Key == "error" && (string)t.Value == string.Empty);
}
else
{
Assert.DoesNotContain(zipkinSpan.Tags, t => t.Key == "error");
}
}
}
}
108 changes: 104 additions & 4 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 @@ -192,7 +192,7 @@ public void ErrorGettingUriFromEnvVarSetsDefaultEndpointValue()
[InlineData(false, false, false, StatusCode.Ok, null, true)]
[InlineData(false, false, false, StatusCode.Error)]
[InlineData(false, false, false, StatusCode.Error, "Error description")]
public void IntegrationTest(
public void IntegrationTest_With_Status_Tags(
bool useShortTraceIds,
bool useTestResource,
bool isRootSpan,
Expand Down Expand Up @@ -275,21 +275,121 @@ public void IntegrationTest(
switch (statusCode)
{
case StatusCode.Ok:
statusTag = $@"""{SpanAttributeConstants.StatusCodeKey}"":""OK"",";
statusTag = $@",""{SpanAttributeConstants.StatusCodeKey}"":""OK""";
break;
case StatusCode.Unset:
statusTag = string.Empty;
break;
case StatusCode.Error:
statusTag = $@"""{SpanAttributeConstants.StatusCodeKey}"":""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"":{{{resourceTags}""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}}}}}]",
$@"[{{""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"":{{{resourceTags}""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/""{statusTag}{errorTag}}}}}]",
Responses[requestId]);
}

[Theory]
[InlineData(true, false, false)]
[InlineData(false, false, false)]
[InlineData(false, true, false)]
[InlineData(false, false, true)]
[InlineData(false, false, false, ActivityStatusCode.Ok)]
[InlineData(false, false, false, ActivityStatusCode.Ok, null, true)]
[InlineData(false, false, false, ActivityStatusCode.Error)]
[InlineData(false, false, false, ActivityStatusCode.Error, "Error description")]
public void IntegrationTest_With_Activity_Status(
bool useShortTraceIds,
bool useTestResource,
bool isRootSpan,
ActivityStatusCode activityStatusCode = ActivityStatusCode.Unset,
string statusDescription = null,
bool addErrorTag = false)
{
Guid requestId = Guid.NewGuid();

ZipkinExporter exporter = new ZipkinExporter(
new ZipkinExporterOptions
{
Endpoint = new Uri($"http://{this.testServerHost}:{this.testServerPort}/api/v2/spans?requestId={requestId}"),
UseShortTraceIds = useShortTraceIds,
});

var serviceName = (string)exporter.ParentProvider.GetDefaultResource().Attributes
.Where(pair => pair.Key == ResourceSemanticConventions.AttributeServiceName).FirstOrDefault().Value;
var resourceTags = string.Empty;
var activity = CreateTestActivity(isRootSpan: isRootSpan);

activity.SetStatus(activityStatusCode, statusDescription);

if (useTestResource)
{
serviceName = "MyService";

exporter.SetLocalEndpointFromResource(ResourceBuilder.CreateEmpty().AddAttributes(new Dictionary<string, object>
{
[ResourceSemanticConventions.AttributeServiceName] = serviceName,
["service.tag"] = "hello world",
}).Build());
}
else
{
exporter.SetLocalEndpointFromResource(Resource.Empty);
}

if (addErrorTag)
{
activity.SetTag(ZipkinActivityConversionExtensions.ZipkinErrorFlagTagName, "This should be removed.");
}

var processor = new SimpleActivityExportProcessor(exporter);

processor.OnEnd(activity);

var context = activity.Context;

var timestamp = activity.StartTimeUtc.ToEpochMicroseconds();
var eventTimestamp = activity.Events.First().Timestamp.ToEpochMicroseconds();

StringBuilder ipInformation = new StringBuilder();
if (!string.IsNullOrEmpty(exporter.LocalEndpoint.Ipv4))
{
ipInformation.Append($@",""ipv4"":""{exporter.LocalEndpoint.Ipv4}""");
}

if (!string.IsNullOrEmpty(exporter.LocalEndpoint.Ipv6))
{
ipInformation.Append($@",""ipv6"":""{exporter.LocalEndpoint.Ipv6}""");
}

var parentId = isRootSpan ? string.Empty : $@"""parentId"":""{ZipkinActivityConversionExtensions.EncodeSpanId(activity.ParentSpanId)}"",";

var traceId = useShortTraceIds ? TraceId.Substring(TraceId.Length - 16, 16) : TraceId;

string statusTag;
string errorTag = string.Empty;
switch (activityStatusCode)
{
case ActivityStatusCode.Ok:
statusTag = $@",""{SpanAttributeConstants.StatusCodeKey}"":""OK""";
break;
case ActivityStatusCode.Unset:
statusTag = string.Empty;
break;
case ActivityStatusCode.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"":{{{resourceTags}""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/""{statusTag}{errorTag}}}}}]",
Responses[requestId]);
}

Expand Down