-
Notifications
You must be signed in to change notification settings - Fork 440
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
Conversation
09bd144
to
9431dcd
Compare
Codecov ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
9431dcd
to
e025ab0
Compare
bf6b246
to
55114d5
Compare
55114d5
to
cde9019
Compare
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 |
cde9019
to
e2fac73
Compare
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. |
How should this msrv failure be addressed?
I imagine we either pin to a prior compatible tokio version (not my first choice) or bump our msrv to 1.63? |
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.) (Well, after more thought, I can probably answer my own question: We should change it everywhere since we'll now only change Also, for anyone curious about whether bumping msrv is considered a semver change, I found this a useful discussion: |
Yeah we usually just change them all to avoid any gap between crates.
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 |
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 |
Waiting for #1203 to be merged, then I'll rebase and squash these commits on that. |
6d7cde3
to
50cd324
Compare
ced3ab9
to
8cf3074
Compare
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. |
77a9cf6
to
386c5dd
Compare
/easycla |
There was a problem hiding this 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
9adead7
to
b180691
Compare
@TommyCpp to merge this PR, and we can work on actual deprecation as a separate PR, so that this is unblocked. |
b180691
to
52ee479
Compare
52ee479
to
51d932c
Compare
@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. |
Thanks for working on this! |
Fixes #1186
Changes
opentelemetry_sdk
types fromopentelemetry
opentelemetry::sdk
to useopentelemetry_sdk
opentelemetry
that did not pass-thru toopentelemetry_api
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes