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

[exporter] Export instrument library information in Jaeger and Zipkin. #243

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

TommyCpp
Copy link
Contributor

@TommyCpp TommyCpp commented Oct 4, 2020

According to spec open-telemetry/opentelemetry-specification#800 and open-telemetry/opentelemetry-specification#801, We MUST export instrumentation library information for Jaeger and Zipkin exporter.

Also update Jaeger exporter so that the instrumentation version will be set to crate version.

…ent version for Jaeger.

According to spec open-telemetry/opentelemetry-specification#800 and open-telemetry/opentelemetry-specification#801, We MUST export instrumentation library information for Jaeger and Zipkin exporter.

Use crate version as default Jaeger instrumentation library version in consistent with Zipkin.
@TommyCpp TommyCpp marked this pull request as ready for review October 4, 2020 22:12
@TommyCpp TommyCpp requested a review from a team October 4, 2020 22:12
Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

These look good, thanks @TommyCpp. Think it may be good to have an option to disable this in the pipeline builder as well, the values are constant and storage-conscious people may not want to have these on all recorded spans. Can do as a follow up pr later if you prefer.

@TommyCpp
Copy link
Contributor Author

TommyCpp commented Oct 5, 2020

@jtescher That's a good idea! I thought about it but the only concern I have was that spec seems pretty insist to include those in tags. Maybe this worth an issue in specifications?

@jtescher
Copy link
Member

jtescher commented Oct 5, 2020

@TommyCpp I think as long as this is the default it's fine to include an opt-out. For example if you are just using otel to send data to jaeger there is no need to include these if you don't want to as jaeger will not treat these values any differently.

@jtescher
Copy link
Member

jtescher commented Oct 6, 2020

Ok I'll merge this for now, can do the opt-out in a follow up

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.

2 participants