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

proto: bump dependency requirement to match generated code #2168

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

djc
Copy link
Contributor

@djc djc commented Oct 3, 2024

#2159 regenerated the gRPC code using a newer tonic-build version than what this repository currently requires. This resulted in committed generated code that does not compile with the actual minimal requirement as prescribed in Cargo.toml. Fix it and bump the version to prepare for release.

Fixes #2165.

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)

Copy link

codecov bot commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.0%. Comparing base (720c62d) to head (99cdfe1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2168     +/-   ##
=======================================
- Coverage   79.0%   79.0%   -0.1%     
=======================================
  Files        121     121             
  Lines      20945   20945             
=======================================
- Hits       16561   16559      -2     
- Misses      4384    4386      +2     

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

@djc
Copy link
Contributor Author

djc commented Oct 3, 2024

I think it's basically impossible to maintain a sub-1.70 MSRV in the workspace at this point. opentelemetry-proto needs tonic 0.12.3 or newer since #2159, so we can't patch it to an earlier version. I don't see any discussion (either via that PR or via #2140) that makes it clear why y'all are trying to stick with 1.65 anyway, I think that ship has pretty much sailed once Tokio bumped to 1.70.

@cijothomas
Copy link
Member

I think it's basically impossible to maintain a sub-1.70 MSRV in the workspace at this point. opentelemetry-proto needs tonic 0.12.3 or newer since #2159, so we can't patch it to an earlier version. I don't see any discussion (either via that PR or via #2140) that makes it clear why y'all are trying to stick with 1.65 anyway, I think that ship has pretty much sailed once Tokio bumped to 1.70.

AGree. Its time to bump MSRV from 1.65 (which was released in Nov 2022), to newer one (1.70 seems reasonable). IIRC, only reason we are still in 1.65 is because tracing was still supporting 1.63.
@TommyCpp @lalitb @hdost Do you see any concerns with bumping msrv?

@TommyCpp
Copy link
Contributor

TommyCpp commented Oct 3, 2024

IIRC, only reason we are still in 1.65 is because tracing was still supporting 1.63

Agree on bumping to 1.70. We tried to be on par with tracing but that's best effort. The only promise we gave is we will always support last 3 rustc version. Rustc is currently on 1.81 it means as long as we don't bump the MSRC beyond 1.79 we should be good

@lalitb
Copy link
Member

lalitb commented Oct 3, 2024

opentelemetry-sdk and opentelemetry are the more common components used across custom exporters and processors which mayn't have the direct dependencies on the libraries needing the latest rust compiler. So, tried to keep them in sync with tracing and compatible with Debian stable. Also, most of the bump scenarios are coming from tonic/tokio updates, which are optional dependency for these two crates.
But agree, maintaining the separate msrv for different otel crate is getting difficult now.

@djc
Copy link
Contributor Author

djc commented Oct 3, 2024

It might make sense to keep the base crates at a different MSRV, but I think for most purposes Tokio's MSRV is probably conservative enough, and if you want split along crate lines you'll probably have to use different workspaces within the repo or something.

@lalitb
Copy link
Member

lalitb commented Oct 3, 2024

It might make sense to keep the base crates at a different MSRV, but I think for most purposes Tokio's MSRV is probably conservative enough, and if you want split along crate lines you'll probably have to use different workspaces within the repo or something.

Thanks @djc for suggestions. Looking into all aspects, I think it should be best to bump all crates to 1.70. I believe it will also help use cleaning up various version pinning and patching in the repo. If someone would like to contribute, else I can do that once done with internal logging changes.

@djc
Copy link
Contributor Author

djc commented Oct 3, 2024

It might make sense to keep the base crates at a different MSRV, but I think for most purposes Tokio's MSRV is probably conservative enough, and if you want split along crate lines you'll probably have to use different workspaces within the repo or something.

Thanks @djc for suggestions. Looking into all aspects, I think it should be best to bump all crates to 1.70. I believe it will also help use cleaning up various version pinning and patching in the repo. If someone would like to contribute, else I can do that once done with internal logging changes.

Would appreciate it if you can take care of that.

@cijothomas cijothomas mentioned this pull request Oct 3, 2024
@djc djc mentioned this pull request Oct 7, 2024
4 tasks
@cijothomas
Copy link
Member

This can be closed given we are bumping to 1.70 in #2179 ?

@djc
Copy link
Contributor Author

djc commented Oct 7, 2024

This can be closed given we are bumping to 1.70 in #2179 ?

No, as explained in the OP we need to actually bump the tonic dependency for -proto.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -2,7 +2,9 @@

## vNext

- Bump MSRV to 1.70 [#2179](https://github.com/open-telemetry/opentelemetry-rust/pull/2179)
## v0.26.1
Copy link
Member

Choose a reason for hiding this comment

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

I am trying to avoid patches unless really needed. if this is blocking tracing integration, then lets do the patch release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am blocking the tracing integration because this is just broken and I don't want to deal with users complaining about it.

Why don't you want to do patch releases?

Copy link
Member

Choose a reason for hiding this comment

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

Why don't you want to do patch releases?

Don't want to do it unless it is really something that should be patched. Like a regression in the last release or security hot fix etc.

We are doing a release every ~3 weeks now for next few months (until we reach stable). Any normal changes can be shipped as part of the regular release itself.

If this is a blocker for tracing, happy to make an exception and do the patch.

@cijothomas cijothomas merged commit 99d24b7 into open-telemetry:main Oct 8, 2024
25 checks passed
@djc
Copy link
Contributor Author

djc commented Oct 8, 2024

Yes, it is a blocker.

Seems like a lot of churn to do semver-incompatible releases every 3 weeks, especially if you're going to do semver-incompatible releases only for the sake of keeping all the versions in sync.

@cijothomas
Copy link
Member

@lalitb will push patch for this shortly.

@cijothomas
Copy link
Member

Yes, it is a blocker.

Seems like a lot of churn to do semver-incompatible releases every 3 weeks, especially if you're going to do semver-incompatible releases only for the sake of keeping all the versions in sync.

Good point! However, we are nearing RC stage, after which we don't expect breaking changes. So it should be simple upgrades. If we experience otherwise, we can adjust the velocity accordingly.

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.

no associated item named GRPC_STATUS found for struct tonic::Status
4 participants