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 into
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
|
@DmitryAstafyev Can you share bit more about the intended use-case? Since OTLP Exporter override scope-name with target, this may not have the intended effect, so want to make sure we have a good grasp of the scenario. |
Thanks for your question @cijothomas . I hope I've gotten your question in right way. We are interested in not loosing attributes, overwriting scope-name with target is okey. The use case is simple: we need to attach per-invocation metadata to every log record's scope in OTLP output. This metadata is dynamic and cannot live on the Resource level, so it belongs on the If the concern is that scope attributes were being discarded by the OTLP exporter when a target is present - that was indeed a real issue, but it appears to have been fixed by 3332 which was merged recently. With both fixes in place the full use case should work as intended for us. |
Thanks for the explanation @DmitryAstafyev! One clarification — the InstrumentationScope is set once when the bridge is created and stays fixed for its lifetime. So by "per-invocation metadata," do you mean per-process-invocation (e.g., a CLI/batch job that runs and exits)? In a long-running server, the scope attributes wouldn't change across requests — per-request data would need to go as log record attributes instead. Once you share more details, I can tell if there is better way to do it.! If you need per-request metadata in a long-running server, |
Thanks @cijothomas for your question. Yes, our use case is per-invocation, not per-request in a long-running server. Sorry for confusion in my prev-reply. For each function invocation we create a fresh OpenTelemetryTracingBridge with invocation-scoped metadata (e.g. invocation ID etc.). Span attributes on individual log records wouldn't be a fit here, since the metadata describes the bridge's operational context rather than individual log events. A native builder_with_scope would let us drop the wrapper-workaround we currently rely on. |
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