Fix MultiplexInstrument tracer fan-out - #235
Conversation
**Motivation:**\nA MultiplexInstrument forwards context propagation to all instruments, but InstrumentationSystem selected only the first tracer for span creation.\n\n**Modifications:**\nAdd a package-scoped type-erased MultiplexTracer and MultiplexSpan, forward tracer and span operations to every underlying tracer, and cover both direct and global tracing paths with tests.\n\n**Result:**\nMultiple tracing backends configured through MultiplexInstrument now receive the same span lifecycle operations.
|
|
||
| @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) | ||
| extension InstrumentationSystem { | ||
| private static var multiplexTracer: MultiplexTracer? { |
There was a problem hiding this comment.
Is this really necessary? A multiplex tracer or instrument should really just behave as any other tracer/instrument and should not need any special handling. It should be full on possible to implement the multiplex instrument/tracer outside of this module and the instrument/tracer properties behave correct.
There was a problem hiding this comment.
Thanks for raising this. The reason I put the forwarding type here is that InstrumentationSystem.tracer currently selects only the first tracer when bootstrapped with MultiplexInstrument; without an adapter at this boundary, global tracing calls do not fan out to the other configured tracers. I agree that a multiplex tracer could be implemented outside this module, so I don’t want to assume this is the preferred API boundary. Would you prefer keeping the global accessor behavior unchanged and moving fan-out to an external integration, or should this PR keep the global accessor behavior and add the smallest supported adapter? I’m happy to narrow the patch to the project’s preferred direction.
There was a problem hiding this comment.
Let me clarify. There can only be a single bootstrapped instrument or tracer. The fact that the current implementation checks if that bootstrapped instrument/tracer is a multiplex instrument is in my opinion wrong. Multiplex instruments/tracers are just as a regular instrument/tracer and they must not get any special treatment inside the core library here.
There was a problem hiding this comment.
Understood. I agree that the core tracing accessor should treat a bootstrapped multiplex value like any other tracer and should not inspect MultiplexInstrument specially. The current patch does not meet that boundary: it adds the special-case in InstrumentationSystem+Tracing.swift.
I will not push a speculative API rewrite. MultiplexInstrument lives in the Instrumentation target while Tracer and Span live in Tracing, so making the multiplex value a regular Tracer requires choosing the supported ownership/API boundary for that wrapper. Please confirm whether you want the multiplex tracer abstraction moved into the Instrumentation target or exposed as a separate tracing bootstrap value; once that boundary is confirmed, I can remove the special-case and update the tests accordingly.
| } | ||
|
|
||
| package var context: ServiceContext { | ||
| self.spans[0].context |
There was a problem hiding this comment.
I think this needs to be discussed. Returning just the first context is at least confusing and needs documenting. But I would discuss what should be the expected context for a MultiplexSpan.
|
Agreed — returning the first context is an ambiguous contract, and |
3 participants
Fixes #232
MultiplexInstrument now forwards span creation and span lifecycle operations to every configured tracer, including context propagation, attributes, events, errors, links, status, flush, and end. This adds direct and global instrumentation coverage for multiple tracers.
Validation:
swift test
git diff --check