Support detaching from thread state without creating a new span #770
Conversation
Generate changelog in
|
tracing/src/main/java/com/palantir/tracing/NopDetachedSpan.java
Outdated
Show resolved
Hide resolved
| return sampled | ||
| ? new SampledDetachedSpan(operation, type, traceId, requestId, parentSpan) | ||
| : new UnsampledDetachedSpan(traceId, requestId, parentSpan); | ||
| : new UnsampledDetachedSpan(traceId, requestId, Optional.empty()); |
There was a problem hiding this comment.
This is a simplification for readability: parentSpan is always empty here, but it wasn't obvious reading the code.
| setTrace(newTrace); | ||
| // Do not complete the synthetic root span, it simply prevents nested spans from removing trace state, and | ||
| // allows | ||
| return maybeCurrentTrace == null ? REMOVE_TRACE : () -> Tracer.setTrace(maybeCurrentTrace); |
There was a problem hiding this comment.
These methods are duplicated with the sampled implementation. Once I'm happier with the API I'll revisit this.
| Tracer.setSampler(AlwaysSampler.INSTANCE); | ||
| // Initialize a new trace for each test | ||
| Tracer.setTrace(Trace.of(true, "defaultTraceId", Optional.empty())); | ||
| Tracer.initTraceWithSpan(Observability.SAMPLE, "defaultTraceId", "rootOperation", SpanType.LOCAL); |
There was a problem hiding this comment.
Fixes the test, a sampled trace with zero spans is an illegal state that doesn't exist in the wild.
carterkozak
changed the title
Co-authored-by: Tom Petracca <[email protected]>
| Trace originalTrace = original; | ||
| if (originalTrace != null) { | ||
| Tracer.setTrace(originalTrace); | ||
| } |
There was a problem hiding this comment.
why do we need the originalTrace assignment here?
There was a problem hiding this comment.
oh I see this was just copied from further down in the diff here
There was a problem hiding this comment.
When we apply a trace somewhere else in a deferred way, it's important that it doesn't destroy any existing tracing that's already happening with a potentially (but not necessarily) different traceId.
Basically the tracing state before this is applied should exactly match the state after it's closed.
|
Released 6.2.0 |
Before this PR
spans + deferredtracer.
After this PR
Removed unhelpful spans: executors, runnables, and callables wrapped without an operation name no longer produce spans with the generic name
DeferredTracer(unnamed operation). It's possible that these provided value in some scenarios, however we strongly recommend providing a relevant name instead. These generic spans tend to be unhelpful debugging when things go wrong.Some products trace thousands of items, sampled requests can result in a lot of allocation overhead deep-copying span stacks. DetachedSpan only peeks at one span.
==COMMIT_MSG==
Allow detachment from thread state without creating a new span.
==COMMIT_MSG==
Possible downsides?
This is confusing. The original span may close/end before the detached element, however that's already the case using DeferredTracer. This change allows us to leverage the nicer, more efficient DetachedSpan API as a replacement for DeferredTracer.
I could not update DeferredTracer because it has been used to transfer an entire span stack to another thread, which I do not intend to support with this model.