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

[Docs]: Compatibility matrix #1966

Closed
xd009642 opened this issue Jul 25, 2024 · 11 comments
Closed

[Docs]: Compatibility matrix #1966

xd009642 opened this issue Jul 25, 2024 · 11 comments
Labels
enhancement New feature or request triage:todo Needs to be traiged.

Comments

@xd009642
Copy link
Contributor

Related Problems?

No response

Describe the solution you'd like:

This is very much just a this-crate issue. But the usage of traits from different otel crates is widespread enough that whenever I attempt to upgrade I end up in some compatibility nightmare trying to get all the versions aligned to the correct one. As far as I see if it, the solution would be either to move versions up lockstep and disregarding semver (or at least broadening it's definition to cover all the various crates), or alternatively document for each opentelemetry version what the latest compatible version for the other crates are.

I'd rather not spend another hour of my life trying to solve issues like the trait JaegerTraceRuntimeis not implemented forTokio`` whenever I upgrade this crate. Unfortunately, the opentelemetry upgrades are the most painful upgrades I've found in the Rust ecosystem 😢

Considered Alternatives

No response

Additional Context

No response

@xd009642 xd009642 added enhancement New feature or request triage:todo Needs to be traiged. labels Jul 25, 2024
@lalitb
Copy link
Member

lalitb commented Jul 25, 2024

solution would be either to move versions up lockstep and disregarding semver (or at least broadening it's definition to cover all the various crates), or alternatively document for each opentelemetry version what the latest compatible version for the other crates are.

@xd009642 In the current publishing process, we coordinate the versions of all OpenTelemetry-related crates to be updated simultaneously, ensuring compatibility across the board. The tracing-opentelemetry crate is an exception as it is part of tokio ecosystem, and so is upgraded separately afterwards. This could bring the is not implemented for errors in interim. While discussions around #1571 are still ongoing, I agree that there needs to be better synchronization to address these issues.

@ibigbug
Copy link

ibigbug commented Jul 29, 2024

vote up for this.

I'm still facing trait issue when every individual package is up to date

# opentelemetry
opentelemetry = "0.24"
opentelemetry_sdk = { version = "0.24.1", features = ["rt-tokio"] }
tracing-opentelemetry = "0.25"
opentelemetry-jaeger-propagator = "0.3"
opentelemetry-otlp = { version = "0.17", features = ["grpc-tonic"] }
error[E0277]: the trait bound `opentelemetry_sdk::trace::TracerProvider: opentelemetry::trace::Tracer` is not satisfied
   --> clash_lib/src/app/logging.rs:119:57
    |
119 |         Some(tracing_opentelemetry::layer().with_tracer(tracer))
    |                                             ----------- ^^^^^^ the trait `opentelemetry::trace::Tracer` is not implemented for `opentelemetry_sdk::trace::TracerProvider`
    |                                             |
    |                                             required by a bound introduced by this call
    |
    = help: the following other types implement trait `opentelemetry::trace::Tracer`:
              BoxedTracer
              NoopTracer
              opentelemetry_sdk::trace::Tracer
note: required by a bound in `OpenTelemetryLayer::<S, T>::with_tracer`
   --> /home/wtf/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tracing-opentelemetry-0.25.0/src/layer.rs:591:17
    |
589 |     pub fn with_tracer<Tracer>(self, tracer: Tracer) -> OpenTelemetryLayer<S, Tracer>
    |            ----------- required by a bound in this associated function
590 |     where
591 |         Tracer: otel::Tracer + PreSampledTracer + 'static,
    |                 ^^^^^^^^^^^^ required by this bound in `OpenTelemetryLayer::<S, T>::with_tracer`

just giving a concrete example

@cijothomas
Copy link
Member

@ibigbug Are you facing issues without tracing-opentelemetry as well? If the issue is when tracing-opentelemetry is involved, then not much we can do in this repo. The final answer depends on resolving #1571

@xd009642
Copy link
Contributor Author

@cijothomas the thing is there is something you can do, if for every version you release you have a table of the versions of each ecosystem crate that is compatible. We can go back to the crates.io or docs.rs page for the opentelemetry version tracing-opentelemetry is using and see what versions we have to use.

@lalitb
Copy link
Member

lalitb commented Jul 29, 2024

 Some(tracing_opentelemetry::layer().with_tracer(tracer))
    |                                             ----------- ^^^^^^ the trait `opentelemetry::trace::Tracer` is not implemented for `opentelemetry_sdk::trace::TracerProvider`
    |                                             |
    |                                             required by a bound introduced by this call

@ibigbug You need to modify your code as the opentelemetry_otlp::new_pipeline() now returns TracerProvider instead of Tracer. Please follow the example here.

@cijothomas
Copy link
Member

@cijothomas the thing is there is something you can do, if for every version you release you have a table of the versions of each ecosystem crate that is compatible. We can go back to the crates.io or docs.rs page for the opentelemetry version tracing-opentelemetry is using and see what versions we have to use.

I don't support the idea of opentelemetry repo (this repo) documenting the compatibility matrix of other ecosystem crates, in particular tracing-opentelemetry, without formally deciding and closing #1571.

From my observation, I am not even sure if tracing-opentelemetry maintainers are interested in actively reviewing PRs in this repo or they plan to ignore opentelemetry and write an otlp exporter directly
#1937

The current state is highly unacceptable. Once #1571 is resolved, do you still see more compat issues? @TommyCpp and other maintainers (including me), were discussing making an equivalent of tracing-opentelemetry right in this repo's sdk itself

@xd009642
Copy link
Contributor Author

Oh no I just want you to document your own compatibility. opentelemetry_sdk, opentelemetry-semantic-conventions, opentelemetry-otlp, opentelemetry-contrib, opentelemetry, opentelemetry-http etc etc etc. Most of my trait mismatches have came with the interplay between those crates. Matching tracing_opentelemetry to opentelemetry is fairly simple but then going back in time and getting all of those matched takes a lot of effort.

@cijothomas
Copy link
Member

I see.. Thanks for clarifying. Is the below reply from @lalitb good enough? i.e we already release all crates together. (sorry about missing zipkin crate by mistake). Is that not sufficient?

@xd009642 In the current publishing process, we coordinate the versions of all OpenTelemetry-related crates to be updated simultaneously, ensuring compatibility across the board.

Stepping back - is the issue really because we are breaking things in every release? (Something we have an end in sight!)

@xd009642
Copy link
Contributor Author

I find the issue personally, is the trait impedance mismatch in rust. So as some of the core traits may be reimplemented in different places and then consumed by other traits the error messages don't necessarily point me to the crate causing the actual issue. The APIs may be otherwise compatible but monomorphisation is more fussy than a programmer

@TommyCpp
Copy link
Contributor

TommyCpp commented Sep 9, 2024

close with #2084

@TommyCpp TommyCpp closed this as completed Sep 9, 2024
@cijothomas
Copy link
Member

@xd009642 Please let us know if you have more suggestions on this. We just unified the version across all crates in this repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage:todo Needs to be traiged.
Projects
None yet
Development

No branches or pull requests

5 participants