Make active span be ignored by default; used as parent span if configured#110
Conversation
| @Override | ||
| protected boolean shouldUseParentSpan() { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Each of the existing IT classes is basically duplicated for the additional scenario, with the shouldUseParentSpan() method controlling:
- whether or not the new opt-in parameter is set, and
- what the assertion looks for.
| protected abstract boolean shouldUseParentSpan(); | ||
|
|
||
| @Override | ||
| protected void initTracing(ServletContextHandler context) { |
There was a problem hiding this comment.
This method is copied from the AbstractServerDefaultConfigurationTest, but with some logic to control whether or not we want to use the new withJoinExistingActiveSpan method in the builder.
| * Default is false. | ||
| * @return builder | ||
| */ | ||
| public Builder withJoinExistingActiveSpan(boolean joinExistingActiveSpan) { |
There was a problem hiding this comment.
Are you happy with the naming of this? I couldn't think of much better, but perhaps it could be improved.
| if (tracer != null) { | ||
|
|
||
| Tracer.SpanBuilder spanBuilder = tracer.buildSpan(operationNameProvider.operationName(requestContext)) | ||
| .ignoreActiveSpan() |
There was a problem hiding this comment.
This could probably always have been in the code as far as I can tell; the thing that actually seems to be more influential is:
if (parentSpanContext != null) {
spanBuilder.asChildOf(parentSpanContext);
}
below.
rnorth
force-pushed
the
configurable-active-span-joining
branch
from
1a07cee to
40ab1aa
Compare
…ured. The ensures that the damage caused by accidental thread local leakage between requests is minimized Fixes opentracing-contrib#109
rnorth
force-pushed
the
configurable-active-span-joining
branch
from
40ab1aa to
6559b80
Compare
|
Hmm, I pushed some changes to my branch but they're not appearing. Will try closing and reopening... |
|
Hmm, I pushed some changes to my branch but they're not appearing in the PR. Will try to close and reopen... |
|
Ah never mind - GitHub is having issues at the moment: https://status.github.com/messages |
|
It seems that yesterday's GitHub consistency issues are now resolved and this is showing the correct content. Ready for review! |
|
What is the idea of this change, that active span are ignored by default ? I think the active span leakage could better be solved by something such as opentracing/opentracing-java#313 |
|
Hi @sjoerdtalsma I would wholeheartedly say that opentracing/opentracing-java#313 ought to happen regardless - being able to clear the state at the end of handling a request is important and feels like a missing part of the API (it was the first thing I looked for when tackling this problem TBH) Regarding this PR, though, I'd say that perhaps we should do both - tackle the problem at both ends. Clean up after ourselves, but don't assume the previous occupant of the thread did the same. Turning things around, I don't really see a compelling use case to re-use active spans by default. There is the servlet filter + jersey filter scenario, but I'd argue that it's pretty niche, and the current model of not even being able to opt-out feels wrong. If it helps, I'd be totally fine with modifying this PR to keep the current default behaviour unchanged and allow opt-out. @pavolloffay suggested changing the default but maybe he doesn't have particularly strong views on this. |
pavolloffay
left a comment
pavolloffay
left a comment
There was a problem hiding this comment.
Thanks, It looks good just a couple of minor comments.
| } | ||
|
|
||
| /** | ||
| * @param joinExistingActiveSpan If true, any active span on the current scope will be used as the parent span. |
There was a problem hiding this comment.
on the current thread. This can be used when combining with servlet instrumentation.
|
|
||
| /** | ||
| * @param joinExistingActiveSpan If true, any active span on the current scope will be used as the parent span. | ||
| * If false, and a parent span can be extracted from the HTTP request, that span |
There was a problem hiding this comment.
If false parent span will be extracted from HTTP headers.
| if (shouldUseParentSpan()) { | ||
| Assert.assertEquals(preceding.context().spanId(), original.parentId()); | ||
| } else { | ||
| Assert.assertNotEquals(preceding.context().spanId(), original.parentId()); |
There was a problem hiding this comment.
You can equal to 0 which means no parent
The problem is that we don't if the previous layer forgot to clear the context or it's indicating to join the trace. |
No problem if you clear after each request I think. Thread is returned to the threadpool anyway, so must be clear of lingering Spans. |
|
I think it might be a problem, the previous layer which set the context is responsible of clearing it. |
I'm afraid I'm wary of that assertion - the thread certainly should be free from dirty threadlocal state, but mistakes could happen. The current behaviour seems to only be useful in quite niche cases. In the majority of cases it's not just unnecessary, it's potentially dangerous.
This is a really good point; when it's available, opentracing/opentracing-java#313 will need to be configurable (opt-out?) so that the JAX-RS filter doesn't nuke the scope that an outer servlet filter is actually responsible for closing. |
|
thanks @rnorth |
3 participants
The ensures that the damage caused by accidental thread local leakage between requests is minimized.
Fixes #109