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

Move 3rd party propagators and merge exporter into sdk::export #375

Merged
merged 11 commits into from Dec 7, 2020

Conversation

TommyCpp
Copy link
Contributor

@TommyCpp TommyCpp commented Dec 2, 2020

Fix #361
Fix #340

This PR:

  • Moved 3rd party propagators into opentelemetry-contrib/propagator
  • Added testing feature so that we could share test util between crates.
  • Moved exporter into sdk::export
  • Updated the opentelemetry-proto

Upgrade note:

  • For aws propagators. Add opentelemetry-contrib in your Cargo.toml. Choose the feature based on the propagators you need and import them via opentelemetry_contrib::trace::propagators::<propagators name>.
  • For types from exporter module. Such as ExportError. Change the import path from opentelemetry::exporter into opentelemetry::sdk::export
  • For jaeger propagators. Add opentelemetry-jaeger in your Cargo.toml. use opentelemetry_jaeger::Propagator
  • For b3 propagators. Add opentelemetry-zipkin in your Cargo.toml. use opentelemetry_zipkin::Propagator

Open question(s):

  • Where should we put the jaeger and b3 propagtor? It's intutive to me that we include them as part of the opentelemetry-jaeger or opentelemetry-zipkin crate. But Go impl put it in contrib Moved zipkin and jaeger propagators into their own crate.

@codecov
Copy link

codecov bot commented Dec 2, 2020

Codecov Report

Merging #375 (a8c9685) into master (a0ade71) will decrease coverage by 4.80%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
- Coverage   53.93%   49.13%   -4.81%     
==========================================
  Files          71       66       -5     
  Lines        6081     5326     -755     
==========================================
- Hits         3280     2617     -663     
+ Misses       2801     2709      -92     
Impacted Files Coverage Δ
opentelemetry-otlp/src/span.rs 2.67% <0.00%> (ø)
opentelemetry/src/api/metrics/mod.rs 0.00% <ø> (ø)
opentelemetry/src/api/trace/mod.rs 0.00% <ø> (ø)
opentelemetry/src/api/trace/noop.rs 68.26% <ø> (ø)
opentelemetry/src/lib.rs 100.00% <ø> (ø)
opentelemetry/src/sdk/export/metrics/mod.rs 0.00% <ø> (ø)
opentelemetry/src/sdk/export/metrics/stdout.rs 0.00% <ø> (ø)
opentelemetry/src/sdk/export/trace/mod.rs 56.52% <ø> (ø)
opentelemetry/src/sdk/export/trace/stdout.rs 17.64% <ø> (ø)
opentelemetry/src/sdk/trace/provider.rs 75.51% <ø> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0ade71...a8c9685. Read the comment docs.

@TommyCpp TommyCpp marked this pull request as ready for review December 2, 2020 16:57
@TommyCpp TommyCpp requested a review from a team as a code owner December 2, 2020 16:57
@jtescher
Copy link
Member

jtescher commented Dec 5, 2020

Hm yeah I think it may be more intuitive to have the jaeger / be propagators with the exporters in their crates rather than contrib.

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.

Other than that I think this looks good

@@ -129,3 +129,7 @@ See the [code owners](CODEOWNERS) file.

See the [community membership document in OpenTelemetry community
repo](https://github.com/open-telemetry/community/blob/master/community-membership.md).

## FAQ
### Where should I put third party propagators/exporters, contrib or standalone crates?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wanted to clarify the problem that where to put the stuff here. Please let me know if the below descriptions makes sense. @jtescher

Copy link
Member

Choose a reason for hiding this comment

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

Seems good 👍

@jtescher jtescher merged commit 5192fba into open-telemetry:master Dec 7, 2020
@TommyCpp TommyCpp deleted the move branch December 7, 2020 00:35
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.

Move 3rd party propagators to contrib Consider moving exporters module to sdk
2 participants