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

Update status serialization for Zipkin (was: Remove mention of gRPC status codes) #1186

Merged
merged 3 commits into from
Nov 9, 2020

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Nov 3, 2020

Span.Status is not related to gRPC codes anymore.

`Span.Status` is not related to gRPC codes anymore
@iNikem iNikem requested review from a team November 3, 2020 12:14
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Nice catch!

@@ -138,7 +138,8 @@ TBD: add examples

### Status

Span `Status` MUST be reported as a key-value pair in `tags` to Zipkin.
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.
Copy link
Member

Choose a reason for hiding this comment

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

I think we still want to report a non-empty message even if the code is UNSET. But someone knowledgeable about Zipkin should probably review.

Copy link
Member

Choose a reason for hiding this comment

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

No reason to do that, unnecessary bytes on the wire.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks!

@Oberon00 Oberon00 changed the title Remove mention of gRPC status codes Update status serialization for Zipkin (was: Remove mention of gRPC status codes) Nov 3, 2020
@Oberon00
Copy link
Member

Oberon00 commented Nov 3, 2020

Since this now is more than editorial, we might want an issue for that, so it can be triaged.

I don't think splitting the non-editorial changes off makes much sense, at least in the current master branch state it should be obvious that the spec is outdated here 😃

@iNikem
Copy link
Contributor Author

iNikem commented Nov 5, 2020

@open-telemetry/technical-committee should I do anything else with this or can it be merged?

@tigrannajaryan
Copy link
Member

@open-telemetry/technical-committee should I do anything else with this or can it be merged?

LGTM. Will be merged after the compulsory 2 day period (unless we need it more urgently for some reason).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants