Map Error Status to error tag in Zipkin & Jaeger #1257

CodeBlanch · 2020-11-23T18:42:20Z

Changes

Added Status mapping to Jaeger which is the same as Zipkin.
Defined mapping of Error Status to error tag for Zipkin & Jaeger.
Added a row in the compliance matrix for the Error mapping.

/cc @cijothomas @yurishkuro @tedsuo @adriancole

…ers.

…or-flag

Oberon00 · 2020-11-23T19:37:29Z

I think we use all-lowercase in all other enums in semantic conventions. So I'd rather keep it as-is or use all-lowercase.

specification/trace/sdk_exporters/zipkin.md

Oberon00

I think this PR needs a better motivation. It is a breaking change after all. Also, it seems that the Zipkin change to error needs polishing.

Oberon00 · 2020-11-24T11:37:36Z

specification/trace/sdk_exporters/zipkin.md

 In the latter case it MUST NOT be reported.

 The following table defines the OpenTelemetry `Status` to Zipkin `tags` mapping.

 | Status|Tag Key| Tag Value |
 |--|--|--|
-|Code | `otel.status_code` | Name of the code, either `OK` or `ERROR`. MUST NOT be set if the code is `UNSET`. |
+|Code | `otel.status_code` | Name of the code, either `Ok` or `Error`. MUST NOT be set if the code is `Unset`. |


How valuable is sending this? We can use it to distinguish OK and UNSET, but ERROR is already marked with the error attribute.

Good question. Definitely redundant to have otel.status_code=ERROR with the error flag also present on either Jaeger or Zipkin. But having otel.status_code does keep things consistent and allows for easier translation to/from OTLP? Might also be better to leave with both in there for now so this is an additive change and not a breaking one?

CodeBlanch · 2020-11-24T17:47:47Z

For motivation, check out open-telemetry/opentelemetry-dotnet#1579. There are 2 screenshots on there that show what Zipkin & Jaeger do with the error flag. Basically, the UI highlights the spans as errors when the flag is present. There's also some stuff that shows on the search results, but I don't have a screenshot handy for that.

Oberon00 · 2020-11-24T17:52:37Z

Sorry, my complaint was worded ambiguous: I am in favor of adding error when the status equals ERROR.

I meant a motivation to change the casing of otel.status_code.

CodeBlanch · 2020-11-24T18:14:39Z

I think we use all-lowercase in all other enums in semantic conventions. So I'd rather keep it as-is or use all-lowercase.

I put them back to uppercase.

Oberon00

Thank you!

anuraaga

One small clarification comment but generally looks good

anuraaga · 2020-11-25T01:48:25Z

specification/trace/sdk_exporters/zipkin.md

+### Error flag
+
+When Span `Status` is set to `ERROR` an `error` tag SHOULD be added with an
+empty string value (eg. `{"error":""}`). The added `error` tag MAY override any


I'm a bit confused on the "override previous value" - does it mean that if the user set an error tag themselves, this may unset it if Status is UNSET? Or if a user sets error=cats, we override to error=? Just want to have an idea (maybe we can add some clarification)

I was trying to say that exporters can simply add the error tag on the end of the tag list which will effectively override anything already there (with the same error key). My motivation for that was 1) I didn't want to leave it undefined and 2) checking for an existing tag is particularly expensive (O(n)) in the .NET implementation so I was hoping to avoid needing to do that. Open to suggestions for change or improvement.

Why are we setting an error tag with empty string value and additionally add otel.status_description instead of just setting the error tag with a string value equal to Status message?

Doesn't Status message fit nicely in the error tag?

@tigrannajaryan The only issue with that is error tag must ONLY be sent if there was an actual error. So what we would need to do is set error instead of otel.status_description when otel.status_code=ERROR otherwise send otel.status_description (if it has a value for OK or UNSET cases). I don't have strong feelings either way. I will say that as-is is easier to implement (always sending otel.status_description when it has a value and then adding the error tag (empty) for otel.status_code=ERROR) and it's non-breaking.

I believe Status.message is not supposed to be set for OK or UNSET cases. We should explicitly call it out in the spec. This means error tag will only be set equal to Spatus.message when Status.status==ERROR and for all other statuses there is no error message to record.

@tigrannajaryan I added some text on Status to clarify Description should only be used with Error and then updated Zipkin & Jaeger based on that.

Just verifying that the otel-java code follows the specification and I've given the above a read and may have noticed a slight difference between the jaeger and zipkin exporter specs regarding the error Status.

In the jaeger specification it says

otel.status_description : Description of the Status if it has a value otherwise not set.

So this means that for jaeger it sets the otel.status_description if the status description is set - it does not mandate that it is ONLY set if there is an error. Additionally, if the status is an error, it shall add an error tag.

However for zipkin it says the following:

error : Description of the Status. MUST be set if the code is ERROR, use an empty string if Description has no value. MUST NOT be set for OK and UNSET codes.

This describes that the otel.status_description tag isn't set but instead the error tag is the only thing that is set providing that there is an ERROR code and not OK or UNSET.

Is there a reason why for jaeger we provide the otel.status_code, otel.status_description AND the error tag but for zipkin we only provide otel.status_code and the error tag.

@ChrisJBurns Slight differences in the implementations, basically. Zipkin error is a string where you can put the description. Jaeger error is a bool (true/false) so it has otel.status_description for the description. The Zipkin text is a bit more specific because if you send something like error=OK or error=false to Zipkin, it's still going to treat that span as failed.

codefromthecrypt · 2020-11-25T02:58:27Z

In general I'd advise looking at this as it has a lot of information about nuance of where and from who data is collected https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/handler/MutableSpan.java#L37-L87

codefromthecrypt · 2020-12-03T23:50:45Z

specification/trace/sdk_exporters/zipkin.md

@@ -127,9 +127,7 @@ convention](../semantic_conventions/README.md)
 document maps to the strongly-typed fields of Zipkin spans.

 Primitive types MUST be converted to string using en-US culture settings.
-Boolean values must use lower case strings `"true"` and `"false"`, except an


this special case is still true. For example, you have an opentracing bridge which has in the past littered folks stuff with "error"->"false" data. can you please verify that false should never be sent as a value of "error" regardless of if being mapped from the grpc status or not?

OK I added it back with different words as part of the Status section.

tigrannajaryan

Please add this change to the changelog.

CodeBlanch · 2020-12-07T18:05:01Z

@tigrannajaryan CHANGELOG updated.

tigrannajaryan · 2020-12-08T18:00:48Z

Let's keep this open until tomorrow since there were substantial changes. If there are no further comments we can merge tomorrow.

yurishkuro · 2020-12-08T18:34:54Z

specification/trace/api.md

+- An API to set the `Status`. This SHOULD be called `SetStatus`. This API takes
+  the `StatusCode`, and an optional `Description`, either as individual
+  parameters or as an immutable object encapsulating them, whichever is most
+  appropriate for the language. `Description` MUST be IGNORED for `StatusCode`


either as individual parameters or as an immutable object encapsulating them, whichever is most appropriate for the language.

This sounds like a general guideline that would apply to any API function, we don't need to be cluttering individual function descriptions with it.

That was there before my addition you want me to update here?

could be done in another PR

specification/trace/sdk_exporters/jaeger.md

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

specification/trace/sdk_exporters/jaeger.md

CodeBlanch added 2 commits November 23, 2020 10:18

Added mapping of Error Status to error flag in Zipkin & Jaeger export…

d283999

…ers.

Merge remote-tracking branch 'upstream/master' into zipkin-jaeger-err…

2133e6b

…or-flag

CodeBlanch requested review from a team as code owners November 23, 2020 18:42

github-actions bot assigned arminru Nov 23, 2020

CodeBlanch changed the title ~~Map Error Status to error tag in Zipkin & Jaeger~~ Map Error Status to error tag in Zipkin & Jaeger Nov 23, 2020

yurishkuro approved these changes Nov 23, 2020

View reviewed changes

cijothomas approved these changes Nov 23, 2020

View reviewed changes

jmacd approved these changes Nov 24, 2020

View reviewed changes

codefromthecrypt reviewed Nov 24, 2020

View reviewed changes

specification/trace/sdk_exporters/zipkin.md Outdated Show resolved Hide resolved

Oberon00 requested changes Nov 24, 2020

View reviewed changes

Oberon00 reviewed Nov 24, 2020

View reviewed changes

CodeBlanch added 2 commits November 24, 2020 09:53

Zipkin error flag is now an empty string.

4d618e2

Restored uppercase for status enum values.

cd5c3c7

Oberon00 approved these changes Nov 24, 2020

View reviewed changes

Merge branch 'master' into zipkin-jaeger-error-flag

1ea97b2

CodeBlanch mentioned this pull request Nov 25, 2020

Exporter spec fixup 2 open-telemetry/opentelemetry-dotnet#1620

Merged

1 task

anuraaga approved these changes Nov 25, 2020

View reviewed changes

CodeBlanch added 2 commits November 24, 2020 19:41

Removed the error text under attributes.

1fba8f2

Casing fix.

f3d4176

anuraaga mentioned this pull request Nov 27, 2020

Remove Status.value open-telemetry/opentelemetry-java#2147

Merged

ericmustin mentioned this pull request Dec 2, 2020

Handling the ZipkinV2 span error tag conversion to OTLP open-telemetry/opentelemetry-collector#2214

Closed

CodeBlanch added 2 commits December 3, 2020 12:37

Merging latest from master.

f16cc91

Review feedback.

b62b651

codefromthecrypt reviewed Dec 3, 2020

View reviewed changes

Added back information about removing error flag when false.

8534795

ericmustin mentioned this pull request Dec 4, 2020

fix zipkinv2 translation error tag handling open-telemetry/opentelemetry-collector#2253

Merged

tigrannajaryan approved these changes Dec 7, 2020

View reviewed changes

CodeBlanch added 2 commits December 7, 2020 10:00

Merge branch 'master' into zipkin-jaeger-error-flag

0bf7fa8

Updated CHANGELOG.

91f9c1f

yurishkuro approved these changes Dec 8, 2020

View reviewed changes

CodeBlanch and others added 2 commits December 8, 2020 22:36

Update specification/trace/sdk_exporters/jaeger.md

70fd808

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

Merge branch 'master' into zipkin-jaeger-error-flag

de9a65f

yurishkuro reviewed Dec 9, 2020

View reviewed changes

specification/trace/sdk_exporters/jaeger.md Outdated Show resolved Hide resolved

CodeBlanch added 2 commits December 10, 2020 11:22

Merge branch 'master' into zipkin-jaeger-error-flag

a6ebf3d

Review feedback.

2891d74

yurishkuro approved these changes Dec 10, 2020

View reviewed changes

yurishkuro merged commit 0e25dd5 into open-telemetry:master Dec 10, 2020

lzchen mentioned this pull request Dec 10, 2020

Update StatusCodes enum to comply with specs and another opentelemetry libraries. open-telemetry/opentelemetry-python#1402

Closed

jkwatson mentioned this pull request Dec 10, 2020

Verify and update jaeger and zipkin exporters for error code handling. open-telemetry/opentelemetry-java#2272

Closed

jtescher mentioned this pull request Dec 11, 2020

Set status tag in Zipkin & Jaeger open-telemetry/opentelemetry-rust#380

Closed

CodeBlanch mentioned this pull request Dec 11, 2020

Status & error flag updates to match spec open-telemetry/opentelemetry-dotnet#1655

Merged

1 task

matej-g mentioned this pull request Dec 11, 2020

Update span status & description setting to comply with the spec open-telemetry/opentelemetry-go#1376

Closed

srikanthccv mentioned this pull request Dec 16, 2020

Update Jaeger exporter status code to error tag open-telemetry/opentelemetry-python#1474

Closed

CodeBlanch deleted the zipkin-jaeger-error-flag branch March 15, 2021 22:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map Error Status to error tag in Zipkin & Jaeger #1257

Map Error Status to error tag in Zipkin & Jaeger #1257

CodeBlanch commented Nov 23, 2020 •

edited

Loading

Oberon00 commented Nov 23, 2020

Oberon00 left a comment

Oberon00 Nov 24, 2020

CodeBlanch Nov 24, 2020

CodeBlanch commented Nov 24, 2020

Oberon00 commented Nov 24, 2020 •

edited

Loading

CodeBlanch commented Nov 24, 2020

Oberon00 left a comment

anuraaga left a comment

anuraaga Nov 25, 2020

CodeBlanch Nov 25, 2020

tigrannajaryan Dec 2, 2020

CodeBlanch Dec 2, 2020

tigrannajaryan Dec 2, 2020

CodeBlanch Dec 3, 2020

ChrisJBurns Dec 15, 2020 •

edited

Loading

CodeBlanch Dec 15, 2020

codefromthecrypt commented Nov 25, 2020

codefromthecrypt Dec 3, 2020

CodeBlanch Dec 4, 2020

tigrannajaryan left a comment

CodeBlanch commented Dec 7, 2020

tigrannajaryan commented Dec 8, 2020

yurishkuro Dec 8, 2020

CodeBlanch Dec 9, 2020

yurishkuro Dec 9, 2020

Map Error Status to error tag in Zipkin & Jaeger #1257

Map Error Status to error tag in Zipkin & Jaeger #1257

Conversation

CodeBlanch commented Nov 23, 2020 • edited Loading

Changes

Oberon00 commented Nov 23, 2020

Oberon00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented Nov 24, 2020

Oberon00 commented Nov 24, 2020 • edited Loading

CodeBlanch commented Nov 24, 2020

Oberon00 left a comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisJBurns Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Nov 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan left a comment

Choose a reason for hiding this comment

CodeBlanch commented Dec 7, 2020

tigrannajaryan commented Dec 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented Nov 23, 2020 •

edited

Loading

Oberon00 commented Nov 24, 2020 •

edited

Loading

ChrisJBurns Dec 15, 2020 •

edited

Loading