-
Notifications
You must be signed in to change notification settings - Fork 600
feat(otlp): Re-export tonic crate #2898
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
feat(otlp): Re-export tonic crate #2898
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2898 +/- ##
=====================================
Coverage 81.3% 81.3%
=====================================
Files 126 126
Lines 24294 24294
=====================================
Hits 19774 19774
Misses 4520 4520 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@MathieuTricoire Re-exporting the whole tonic crate works, but it exposes all of tonic’s API as part of our public API. If we only need a few types (like |
|
@lalitb Thanks for the answer! That makes sense, and I'm totally good with cherry-picking. I actually thought about that, but I didn't want to overthink it right away if not necessary. Going cherry-picking route, I see two options: Flat re-export: #[cfg(feature = "grpc-tonic")]
pub mod tonic_types {
#[doc(no_inline)]
pub use tonic::metadata::MetadataMap;
#[doc(no_inline)]
#[cfg(feature = "tls")]
pub use tonic::transport::{Certificate, ClientTlsConfig, Identity};
}Grouped by module: #[cfg(feature = "grpc-tonic")]
pub mod tonic_types {
pub mod metadata {
#[doc(no_inline)]
pub use tonic::metadata::MetadataMap;
}
#[cfg(feature = "tls")]
pub mod transport {
#[doc(no_inline)]
pub use tonic::transport::{Certificate, ClientTlsConfig, Identity};
}
}I kind of like the module-based one, feels cleaner and easier to extend later to me, but maybe over-engineered? Happy to go with whichever makes more sense for you. Also wondering what you think about the naming, would Also are you ok with the current re-export scope? Let me know what direction you'd prefer and I'll adjust the PR 👍 |
|
I pushed the grouped by module proposal, let me know your thoughts. |
|
thanks @MathieuTricoire - I agree - the module-based structure feels cleaner and more future-proof, especially if we add more exports later. So let's go with that. |
bantonsson
left a comment
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.
👍🏻 on the module based approach
| "tonic::transport::tls::Identity", | ||
| "tonic::transport::channel::Channel", | ||
| "tonic::transport::error::Error", | ||
| "tonic::service::interceptor::Interceptor", |
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.
I assume that all of these should be reexported and removed?
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.
Hi @bantonsson, thanks for the review!
The added types are the ones now re-exported.
I also updated the CI script to check allowed external types with all features enabled, because the previous configuration was correct but wasn't actually enforced (some re-exported types are feature-gated).
I've also cleaned up the old types that are no longer re-exported.
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.
Thanks for the explanation @MathieuTricoire. Didn't know how that plugin works.
| "tonic::transport::tls::Identity", | ||
| "tonic::transport::channel::Channel", | ||
| "tonic::transport::error::Error", | ||
| "tonic::service::interceptor::Interceptor", |
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.
Thanks for the explanation @MathieuTricoire. Didn't know how that plugin works.
|
CI has been fixed after the merge of #2945 and the PR should be ready for merging. I have checked the |
cijothomas
left a comment
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.
Looks good, can you add a changelog entry too?
|
Hi @cijothomas, I've added the changelog entry, let me know if that works for you 👍 |
Fixes #882
Design discussion issue
I chose to re-export the entire
toniccrate instead of cherry-picking types to avoid maintenance overhead. The re-export is gated behind thegrpc-tonicfeature.Let me know your thoughts if re-exporting the whole
toniccrate is too much and how we should tackle this in this situation.Changes
Re-exports the entire
toniccrate from theopentelemetry-otlpcrate, to allows downstream crates to usetonictypes (such asMetadataMap) compatible withopentelemetry-otlpwithout needing to manually match the internaltonicversion.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes