search

opentracing-contrib / java-jaxrs

OpenTracing Java JAX-RS instrumentation
Apache License 2.0
37 stars 33 forks source link

Make active span be ignored by default; used as parent span if configured #110

Closed rnorth closed 6 years ago

rnorth commented 6 years ago

The ensures that the damage caused by accidental thread local leakage between requests is minimized.

Fixes #109

rnorth commented 6 years ago

Hmm, I pushed some changes to my branch but they're not appearing. Will try closing and reopening...

rnorth commented 6 years ago

Hmm, I pushed some changes to my branch but they're not appearing in the PR. Will try to close and reopen...

rnorth commented 6 years ago

Ah never mind - GitHub is having issues at the moment: https://status.github.com/messages

rnorth commented 6 years ago

It seems that yesterday's GitHub consistency issues are now resolved and this is showing the correct content. Ready for review!

sjoerdtalsma commented 6 years ago

What is the idea of this change, that active span are ignored by default ? Other than the active span threadlocal leakage issue do you have a compelling use-case for this?

I think the active span leakage could better be solved by something such as https://github.com/opentracing/opentracing-java/pull/313

rnorth commented 6 years ago

Hi @sjoerdtalsma Thanks for pointing out opentracing/opentracing-java#313 and linking it together with the issues.

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 commented 6 years ago

I think the active span leakage could better be solved by something such as opentracing/opentracing-java#313

The problem is that we don't if the previous layer forgot to clear the context or it's indicating to join the trace.

sjoerdtalsma commented 6 years ago

I think the active span leakage could better be solved by something such as opentracing/opentracing-java#313

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.

pavolloffay commented 6 years ago

I think it might be a problem, the previous layer which set the context is responsible of clearing it.

rnorth commented 6 years ago

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'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.

I think it might be a problem, the previous layer which set the context is responsible of clearing it.

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.

pavolloffay commented 6 years ago

thanks @rnorth