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

Jump to bottom

Add status to the tags for the Zipkin Exporter #1124

Merged
merged 9 commits into from
Sep 18, 2020
Merged

Add status to the tags for the Zipkin Exporter #1124

merged 9 commits into from
Sep 18, 2020

Conversation

wilguo
Copy link
Contributor

@wilguo wilguo commented Sep 16, 2020 •
edited
Loading

Description

This PR adds status to the tags for the Zipkin Exporter, closing #1111.

The changes have been made following these specifications:
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk_exporters/zipkin.md#status

Fixes #1111

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Ran test file for Zipkin Exporter
tox -e py38-test-exporter-zipkin

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@wilguo wilguo requested a review from a team September 16, 2020 23:03
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 16, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

codeboten
codeboten suggested changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

The PR looks pretty good. It would be great to add a test that confirms the status and description is set when the span status is set to a different value.

@codeboten
Copy link
Contributor

Please add a note to the changelog as well!

@codeboten codeboten added the release:required-for-ga To be resolved before GA release label Sep 17, 2020
@lzchen lzchen self-assigned this Sep 17, 2020
@wilguo wilguo requested a review from codeboten September 17, 2020 18:22
codeboten
codeboten approved these changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments! Looks like you'll need to update the branch before we can merge it.

@codeboten
Copy link
Contributor

Looks like its out of date once again

anuraaga
anuraaga reviewed Sep 18, 2020
View reviewed changes
exporter/opentelemetry-exporter-zipkin/src/opentelemetry/exporter/zipkin/__init__.py Outdated
@@ -183,6 +183,15 @@ def _translate_to_zipkin(self, spans: Sequence[Span]):
"otel.instrumentation_library.version"
] = span.instrumentation_info.version

if span.status is not None:
zipkin_span["tags"][
"ot.status_code"
Copy link

Choose a reason for hiding this comment

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

Hey there - these tags don't quite look correct because they're ot. The referenced spec was added several months ago (open-telemetry/opentelemetry-specification#380) before the acronym was officially finalized as otel. I have sent open-telemetry/opentelemetry-specification#967 to fix this.

I'd recommend either proactively using otel (it's pretty obvious that's what it should be, see lines 180 / 183 in this file) or waiting a bit if you'd like to see the update in the spec merged. As it stands, if this merges it'll need to be updated right away anyways.

@anuraaga
Copy link

@wilguo Also as general advice, it would be great if the PR title has more concrete information about the actual change, rather than being too generic. For example, Adds status to the tags for the Zipkin Exporter would be a great PR title. Otherwise, when going through the list of PRs and commits, it's hard to know what happened at a glance even in a general sense.

@anuraaga anuraaga mentioned this pull request Sep 18, 2020
Merged
@wilguo wilguo changed the title Zipkin exporter issue Add status to the tags for the Zipkin Exporter Sep 18, 2020
@codeboten codeboten merged commit e275d70 into open-telemetry:master Sep 18, 2020
@codeboten
Copy link
Contributor

Sorry @anuraaga, I may have prematurely merged this after seeing it had enough approvals previously. If there are any leftover issues, can you open separate issues to track them?

@anuraaga anuraaga deleted the zipkin-exporter-issue branch September 19, 2020 01:15
@anuraaga
Copy link

The merge looks fine since it was updated to use the new (approved but not quite merged) prefixes :)

@wilguo wilguo mentioned this pull request Sep 21, 2020
Closed
6 tasks
alertedsnake pushed a commit to alertedsnake/opentelemetry-python that referenced this pull request Sep 25, 2020
@wilguo
exporter/zipkin: Add status to the tags for the Zipkin Exporter (open…
f1199ff 
…-telemetry#1124)

* [Issue open-telemetry#1111] Add span status to tags for Zipkin exporter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anuraaga anuraaga Awaiting requested review from anuraaga

@codeboten codeboten Awaiting requested review from codeboten

@lzchen lzchen Awaiting requested review from lzchen

Assignees

@lzchen lzchen

Labels
release:required-for-ga To be resolved before GA release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Zipkin exporter must map status
4 participants
@wilguo @codeboten @anuraaga @lzchen