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 the SDK out of opentelemetry crate, which is now the API only #1199

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

shaun-cox
Copy link
Contributor

@shaun-cox shaun-cox commented Aug 9, 2023

Fixes #1186

Changes

  • remove the re-exports of opentelemetry_sdk types from opentelemetry
  • fix all places that were using opentelemetry::sdk to use opentelemetry_sdk
  • removed features of opentelemetry that did not pass-thru to opentelemetry_api

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Patch coverage: 11.5% and project coverage change: -0.6% ⚠️

Comparison is base (6f7f8bb) 49.3% compared to head (9adead7) 48.7%.
Report is 1 commits behind head on main.

❗ Current head 9adead7 differs from pull request most recent head 51d932c. Consider uploading reports for the commit 51d932c to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            main   open-telemetry/opentelemetry-rust#1199     +/-   ##
=======================================
- Coverage   49.3%   48.7%   -0.6%     
=======================================
  Files        174     169      -5     
  Lines      21043   20328    -715     
=======================================
- Hits       10380    9915    -465     
+ Misses     10663   10413    -250     
Files Changed Coverage Δ
opentelemetry-appender-log/src/lib.rs 0.0% <ø> (ø)
opentelemetry-appender-tracing/src/layer.rs 0.0% <ø> (ø)
opentelemetry-aws/src/lib.rs 92.5% <ø> (ø)
...elemetry-contrib/src/trace/exporter/jaeger_json.rs 0.0% <0.0%> (ø)
...rib/src/trace/propagator/trace_context_response.rs 95.2% <ø> (ø)
opentelemetry-datadog/src/exporter/mod.rs 24.0% <0.0%> (ø)
opentelemetry-datadog/src/lib.rs 89.6% <ø> (ø)
opentelemetry-dynatrace/src/lib.rs 77.9% <ø> (ø)
opentelemetry-dynatrace/src/metric.rs 86.1% <ø> (ø)
opentelemetry-dynatrace/src/transform/metrics.rs 83.4% <ø> (ø)
... and 32 more

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shaun-cox shaun-cox changed the title depend on api or sdk explicitly Move the SDK out of opentelemetry crate, which is now the API only Aug 10, 2023
@shaun-cox shaun-cox marked this pull request as ready for review August 10, 2023 19:42
@shaun-cox shaun-cox requested a review from a team as a code owner August 10, 2023 19:42
@djc
Copy link
Contributor

djc commented Aug 11, 2023

At this point, should we just move all of the opentelemetry_api code back into the opentelemetry directory? Seems simpler to me. (If there is some reason to defer that to a separate PR, that's fine too, but it seems like it could belong here.) If we do that, we shouldn't have this first commit change all the opentelemetry imports to add _api...

@shaun-cox
Copy link
Contributor Author

At this point, should we just move all of the opentelemetry_api code back into the opentelemetry directory?

My vote would be yes, as otherwise there are two choices for which crate to depend on for the api. I would like to leave it for a follow-up PR though.

@shaun-cox
Copy link
Contributor Author

shaun-cox commented Aug 11, 2023

How should this msrv failure be addressed?

error: package `tokio v1.30.0` cannot be built because it requires rustc 1.63 or newer, while the currently active rustc version is 1.60.0
Error: Process completed with exit code 101.

I imagine we either pin to a prior compatible tokio version (not my first choice) or bump our msrv to 1.63?

@djc
Copy link
Contributor

djc commented Aug 11, 2023

Just bump to 1.63, should probably be in a separate PR though.

@shaun-cox
Copy link
Contributor Author

shaun-cox commented Aug 11, 2023

Just bump to 1.63, should probably be in a separate PR though.

I can do this, but would like clarification before I start. (@jtescher, would like your input too.)
There are over 50 places mentioning "1.60". Would you like them all updated to "1.63" (sans the history in CHANGLELOG.md references), or just the crates that depend on tokio that need 1.63? Thanks!

(Well, after more thought, I can probably answer my own question: We should change it everywhere since we'll now only change msrv CI step to test at 1.63, so there would be no way to catch a regression for one of our crates to stay on 1.60.)

Also, for anyone curious about whether bumping msrv is considered a semver change, I found this a useful discussion:
https://www.reddit.com/r/rust/comments/rtp3yg/is_rust_compiler_version_dependency_update_a/

@TommyCpp
Copy link
Contributor

TommyCpp commented Aug 11, 2023

Yeah we usually just change them all to avoid any gap between crates.

Also, for anyone curious about whether bumping msrv is considered a semver change, I found this a useful discussion

I think most of the crates just bump a minor version. We are still pre-GA so we usually just release it as part of the next breaking change. But once we stablize we probably want to adopt similar principle

@jtescher
Copy link
Member

yeah 1.63 seems fine, eventually having api and sdk drift in terms of msrv would be good but simplicity and correctness are probably more important as we head toward 1.0

@shaun-cox shaun-cox marked this pull request as draft August 11, 2023 19:39
@shaun-cox
Copy link
Contributor Author

Waiting for #1203 to be merged, then I'll rebase and squash these commits on that.

@shaun-cox shaun-cox force-pushed the api_sdk_1186 branch 2 times, most recently from 6d7cde3 to 50cd324 Compare August 11, 2023 22:04
@shaun-cox shaun-cox marked this pull request as ready for review August 12, 2023 12:18
@shaun-cox shaun-cox force-pushed the api_sdk_1186 branch 2 times, most recently from ced3ab9 to 8cf3074 Compare August 15, 2023 15:17
@shaun-cox
Copy link
Contributor Author

I rebased after a merge due to #1202, but now there is a left-over "gen-protoc" in zpages that I think is contributing to a build break in this PR that I'll need some help/advice on resolving.

@cijothomas
Copy link
Member

/easycla

Copy link
Contributor

@hdost hdost left a comment

Choose a reason for hiding this comment

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

As mentioned during the SIG we'll disable build of z pages for now

Cargo.toml Outdated Show resolved Hide resolved
@cijothomas
Copy link
Member

@TommyCpp to merge this PR, and we can work on actual deprecation as a separate PR, so that this is unblocked.

@shaun-cox
Copy link
Contributor Author

@TommyCpp, thanks for the zpages fix. I've rebased this and now it all works with zpages included. This PR is ready for final review and merge if everyone is okay with it.

@TommyCpp
Copy link
Contributor

Thanks for working on this!

@TommyCpp TommyCpp added the integration tests Run integration tests label Aug 24, 2023
@TommyCpp TommyCpp merged commit 759556d into open-telemetry:main Aug 24, 2023
13 of 14 checks passed
@shaun-cox shaun-cox deleted the api_sdk_1186 branch August 24, 2023 12:22
@lalitb lalitb mentioned this pull request Oct 20, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration tests Run integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What is the difference between the API and SDK?
8 participants