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.

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

fix(exporter-zipkin): correct status tags names #2519

Merged
merged 3 commits into from
Oct 9, 2021

Conversation

t2t2
Copy link
Contributor

@t2t2 t2t2 commented Oct 4, 2021

Which problem is this PR solving?

As per OpenTelemetry to Zipkin Transformation spec:

Span Status MUST be reported as a key-value pair in tags to Zipkin, unless it is UNSET.
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.
Description 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.

Short description of the changes

Corrected the tag names (ot. -> otel.), check for unset status

For reference: zipkin exporter in otel-java

@t2t2 t2t2 requested a review from a team October 4, 2021 13:58
@codecov
Copy link

codecov bot commented Oct 4, 2021

Codecov Report

Merging #2519 (06d880b) into main (0c7f1c3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2519   +/-   ##
=======================================
  Coverage   93.23%   93.23%           
=======================================
  Files         137      137           
  Lines        5042     5043    +1     
  Branches     1066     1067    +1     
=======================================
+ Hits         4701     4702    +1     
  Misses        341      341           
Impacted Files Coverage Δ
...ges/opentelemetry-exporter-zipkin/src/transform.ts 100.00% <100.00%> (ø)
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 100.00% <100.00%> (ø)

Copy link
Member

@johnbley johnbley left a comment

Choose a reason for hiding this comment

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

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/zipkin.md suggests that for a status of ERROR, the error messages should be in an error tag/attribute. It specifically calls out that it overrides the generic status mapping rule. Would like to see a fix for that and a test covering the error case. See https://github.com/open-telemetry/opentelemetry-swift/pull/232/files for a recent swift change I did that was similar.

@t2t2 t2t2 changed the title fix(exporter-zipkin): Correct status tags names fix(exporter-zipkin): correct status tags names Oct 4, 2021
@t2t2
Copy link
Contributor Author

t2t2 commented Oct 4, 2021

@johnbley Updated to be based on the zipkin specific doc. Should be already covered by should map OpenTelemetry SpanStatus.message to a Zipkin tag test case.


Seems like unrelated rounding fail in test?

@dyladan
Copy link
Member

dyladan commented Oct 4, 2021

Most likely unrelated. I'm running again to see if it is consistent

Copy link
Member

@johnbley johnbley left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks!

@vmarchaud vmarchaud added the bug Something isn't working label Oct 9, 2021
@vmarchaud vmarchaud merged commit 8d43324 into open-telemetry:main Oct 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants