feat(appender-tracing): add builder support for custom instrumentation scope#3428
feat(appender-tracing): add builder support for custom instrumentation scope#3428DmitryAstafyev wants to merge 5 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3428 +/- ##
=====================================
Coverage 83.2% 83.3%
=====================================
Files 128 128
Lines 25048 25092 +44
=====================================
+ Hits 20859 20904 +45
+ Misses 4189 4188 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hello @cijothomas, could you please take a look at this PR? It's my first time contributing here, and I'm ready to correct/change if it's required. Many thanks in advance for review. |
|
The new scope support looks fine, but this changes the public builder API in a breaking way. fn make_builder<P, L>(
provider: &P,
) -> opentelemetry_appender_tracing::layer::OpenTelemetryTracingBridgeBuilder<P, L>
where
P: opentelemetry::logs::LoggerProvider<Logger = L> + Send + Sync,
L: opentelemetry::logs::Logger + Send + Sync,
{
opentelemetry_appender_tracing::layer::OpenTelemetryTracingBridge::builder(provider)
}Can we keep the builder owned while still adding with_scope()? |
aba679c to
bf413c0
Compare
|
Thank you @lalitb for the review and for highlighting this. You are correct: my original proposal introduced a breaking API change by adding a lifetime to the public My goal was to keep configuration cohesive in the builder ( I have updated the implementation to remove the lifetime and keep the builder owned, as requested. The current trade-off is configuration split across two entry points:
This preserves compatibility, but is less uniform than a fully builder-centric API. From an API stability perspective, I also see a risk in a public Would you prefer one of these directions?
|
|
Thanks for the update and the well-thought-out options @DmitryAstafyev. I'd go with your Option 1 - keep The scope isn't a configuration property of the bridge itself - it's a parameter needed alongside the provider to create the underlying logger. Since it must be known at the same time as the provider, a separate also, note that you need to sign CLA before this can be moved further. |
|
Thanks @lalitb, for your response. I've made the requested changes.
Let me know, please, if I should do something else. |
|
Thanks @DmitryAstafyev LGTM. Will approve/merge this once the CLA is signed. |
6684d49 to
1bef051
Compare
…elemetryTracingBridge API
1bef051 to
866bae7
Compare
Changes
This PR adds
OpenTelemetryTracingBridgeBuilder::with_scope()to allow configuring the OpenTelemetryInstrumentationScopeused by the appender logger.The implementation also updates
OpenTelemetryTracingBridgeBuilderto hold the logger provider untilbuild(), so the builder can expose a set of configuration methods:with_scope()with_span_attribute_allowlist()Design Notes
This change introduces a lifetime parameter on
OpenTelemetryTracingBridgeBuilder<'a>. That was done to keep the builder aligned with the existing style, where the bridge is constructed from a provider reference rather than from a pre-created logger.In return, this keeps the API shape cleaner and gives us a more natural builder surface with related configuration methods grouped together.
Compatibility
Technically, adding the lifetime to the public builder type is a breaking API change.
However, this builder API has not yet shipped in a released version, so this does not affect production users of published crates. In practice, the change is limited to the unreleased API surface.
Testing
Added a unit test (
tracing_appender_with_custom_scope) coveringwith_scope()and verifying that the configuredInstrumentationScopeis propagated to emitted logs.Related
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes