Exposes Tracer.currentSpanCustomizer() and Span.customizer() for safety #650
Conversation
Eventhough we made a safe user type `SpanCustomizer`, we didn't provide a safe alternative to `Tracer.currentSpan()`. This led to routine leaking of `brave.Span` to users (even if they need to cast to discover this). While we did have `CurrentSpanCustomizer`, that relied on current context to work. Particularly the new servlet instrumentation uses explicit references, which are cheaper. This adds a couple utilities and uses them: `Tracer.currentSpanCustomizer()` - safer than `currentSpan()` `Span.customizer()` - safely masks lifecycle methods from parsers These are safer as they don't have the ability to abend the span, and have a chance of holding less state (ex NoopSpanCustomizer is constant). Future work will introduce `UnsafeSpan`, which can rely on these methods to ensure higher performance in single-threaded scenarios such as synchronous calls.
|
cc @shakuzen @zeagord @anuraaga @yschimke @wu-sheng @jonathan-lo @marcingrzejszczak |
|
In a number of cases you get a SpanCustomizer (potentially a NoopSpanCustomizer) and pass it down through other methods. Is checking instanceof NoopSpanCustomizer the preferred way to avoid any additional work ala Log4j, isDebugEnabled? |
|
Looks good, I'll adopt it once it lands. |
|
@yschimke I don't mind making NoopSpanCustomizer public type. sadly we can't add an isNoop() method to customizer because it is an interface and we have a lot of <java8 code still |
|
I will make a note for brave5 about customizer should have isNoop regardless |
|
TIOLI, not a blocking issue in any way for me |
|
@yschimke I suspect you won't be the only to ask this, made a javadoc about it. |
|
not sure timeline on follow-up for unsafe.. might be next release but anyway here's the description #651 |
Eventhough we made a safe user type
SpanCustomizer, we didn't providea safe alternative to
Tracer.currentSpan(). This led to routineleaking of
brave.Spanto users (even if they need to cast to discoverthis). While we did have
CurrentSpanCustomizer, that relied on currentcontext to work. Particularly the new servlet instrumentation uses
explicit references, which are cheaper.
This adds a couple utilities and uses them:
Tracer.currentSpanCustomizer()- safer thancurrentSpan()Span.customizer()- safely masks lifecycle methods from parsersThese are safer as they don't have the ability to abend the span, and
have a chance of holding less state (ex NoopSpanCustomizer is constant).
Future work will introduce
UnsafeSpan, which can rely on these methodsto ensure higher performance in single-threaded scenarios such as
synchronous calls.