Exporter spec updates.

[CodeBlanch]

Changes

I noticed updating the exporter compliance matrix in the spec that Jaeger & Zipkin were missing a couple things that the spec has outlined:

• Zipkin bool tags should be true/false not True/False (spec).
• Unset Status should not be sent (spec).
• Jaeger Event.Name should be sent as event tag, if it isn't already present (spec).

TODOs:

• CHANGELOG.md updated for non-trivial changes

[codecov]

Codecov Report

Merging #1609 (e948b45) into master (9f27b4c) will increase coverage by 0.01%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##           master    #1609      +/-   ##
==========================================
+ Coverage   80.59%   80.61%   +0.01%     
==========================================
  Files         242      242              
  Lines        6567     6578      +11     
==========================================
+ Hits         5293     5303      +10     
- Misses       1274     1275       +1     

Impacted Files	Coverage Δ
....Jaeger/Implementation/JaegerActivityExtensions.cs	95.55% <88.88%> (-0.57%)	⬇️
...plementation/ZipkinActivityConversionExtensions.cs	98.75% <100.00%> (+0.04%)	⬆️
...metry.Exporter.Zipkin/Implementation/ZipkinSpan.cs	95.60% <100.00%> (+0.09%)	⬆️

[cijothomas]

to confirm the full behaviour:
Every status except Unset is send to Zipkin as tags.
Error means, we send additional "error=true" tag. -- (This is not explicitly requested in spec, but is a nice addition)

[CodeBlanch]

@cijothomas Correct. I'm going to open a PR into spec for adding the "error" flag on Zipkin & Jaeger when Status=Error. I don't know if it will get in though because of the freeze?

[cijothomas]

LGTM!
Can you also add changelog entry for both exporters, before merge.

[CodeBlanch]

@cijothomas One other thing I noticed, if you look at Zipkin Status spec it seems to say we should send OK or ERROR not Ok or Error. I didn't update that because I was going to ask you what you wanted to do. The main spec uses Ok and Error so it seems kind of strange to switch them, might just be an oversight in the spec?

[cijothomas]

@reyang @alanwest @eddynaka - Would love to get additional approver eyes as this is non-trivial change, and we are close to 1.0.0.

[reyang]

LGTM.

[cijothomas]

Merging to make part of 1.0.0, as this addresses spec requirement.

[alanwest]

LGTM. Though, one question... I believe it's technically possible for there to be an otel.status_description when status is Unset. I don't think there is supposed to be a description in this scenario, but if there is one, will it get sent? If so, does it make sense to prevent sending the description when status is Unset?

@CodeBlanch

[CodeBlanch]

@alanwest As it is now, otel.status_description will always be sent if it is set to something. The spec doesn't say anything about what to do in that case. I don't think our SDK sets it ever, so that would be user-driven. I feel like if someone set it to something interesting, we should honor it? 🤷

[alanwest]

Make sense. Seems most intuitive to me that if Unset then no description either, but agreed that if the spec does not indicate this, then makes more sense to drive the conversation there rather than here.