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

Update some dependencies #969

Merged
merged 4 commits into from
Mar 9, 2023
Merged

Update some dependencies #969

merged 4 commits into from
Mar 9, 2023

Conversation

djc
Copy link
Contributor

@djc djc commented Feb 13, 2023

Here are some of the easy updates from #922. I left out:

  • All of the examples
  • Updates in -contrib (base64)
  • Updates in -jaeger (thrift, tonic, prost, prost-types)
  • Updates in -otlp (grpc, protobuf)
  • Updates in -proto (protobuf, protobuf-codegen)

For the examples, I think we have way too many example projects to reasonably maintain over time, so I think we should prune back on how many things we want to explicitly support. It would probably help if we move examples related to a particular integration crate into that crate, so that PRs are forced to update related examples. (In many cases, it looks like the example projects mainly duplicate example code from the doctests or tests in the integration crates...) This also conveniently allows me to avoid dealing with unrelated changes suggested in review.

As for grpc and protobuf, it looks like the grpcio maintainers are uninterested in updating grpcio to use protobuf 3. Personally, I think the prost/tonic stack is more attractive in any way, so I question the need for this project to maintain two gRPC integrations in parallel (and am personally not willing to spend time on that).

The base64 crate unfortunately made their API worse in recent releases, and the maintainer is unwilling to improve on that.

@djc djc requested a review from a team February 13, 2023 09:47
@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.2 🎉

Comparison is base (e10344c) 69.7% compared to head (7f6ee06) 69.9%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #969     +/-   ##
=======================================
+ Coverage   69.7%   69.9%   +0.2%     
=======================================
  Files        116     116             
  Lines       9038    8920    -118     
=======================================
- Hits        6307    6243     -64     
+ Misses      2731    2677     -54     
Impacted Files Coverage Δ
opentelemetry-api/src/lib.rs 100.0% <ø> (ø)
opentelemetry-jaeger/src/exporter/collector.rs 20.0% <0.0%> (-0.7%) ⬇️
opentelemetry-jaeger/src/lib.rs 93.3% <ø> (+0.3%) ⬆️
opentelemetry-otlp/src/span.rs 0.0% <ø> (ø)
opentelemetry-zipkin/src/lib.rs 100.0% <ø> (ø)
opentelemetry/src/lib.rs 100.0% <ø> (ø)
opentelemetry-sdk/src/runtime.rs 70.5% <0.0%> (-4.5%) ⬇️
...pentelemetry-sdk/src/metrics/sdk_api/descriptor.rs 72.5% <0.0%> (-3.1%) ⬇️
opentelemetry-api/src/trace/mod.rs 81.8% <0.0%> (-1.2%) ⬇️
opentelemetry-api/src/trace/context.rs 50.0% <0.0%> (-0.9%) ⬇️
... and 32 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hdost
Copy link
Contributor

hdost commented Feb 14, 2023

Perhaps the comments about pruning, could be an issue raised. I would agree that the surface area of the project should be reduced, and I believe I had made similar comments in the past when I mentioned making some of the code build separately. By example moving code into a separate contrib repo (might have been a different suggestion, but the core argument is the same) keep the core well maintained and allow for contribution on but keep support low on non-core components.

@TommyCpp
Copy link
Contributor

TommyCpp commented Feb 21, 2023

I took a stab djc#1 to update some dependencies in some of the creates built on top of the work of this PR.

I second the idea to move the example to their crates and having a contrib repo(#841) to store less core creates. I can open an issue with admins to create a new repo and assign proper access to maintainer/approvers

Looking at the issue on grpcio. It seems they are still open to adding support for proto 3? I am open to removing grpcio support but I don't think we need to figure it out now. I feel like we can open an issue to gather some ideas on if anyone is hard depending on grpcio

@TommyCpp
Copy link
Contributor

djc#2
Bump MSRV to 1.60 as it's required by tonic 0.8

Tonic 0.8 requires MSRV > 1.60
Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@djc
Copy link
Contributor Author

djc commented Mar 6, 2023

Thank you for looking into the MSRV stuff and the other crates!

@TommyCpp TommyCpp merged commit af21bea into open-telemetry:main Mar 9, 2023
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.

3 participants