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

rnorth · 2018-10-17T12:09:42Z

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

Fixes #109

rnorth · 2018-10-17T12:11:58Z

...src/test/java/io/opentracing/contrib/jaxrs2/itest/cxf/ApacheCXFParentSpanResolutionTest.java

+  @Override
+  protected boolean shouldUseParentSpan() {
+    return true;
+  }


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.

rnorth · 2018-10-17T12:13:46Z

...c/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java

+    protected abstract boolean shouldUseParentSpan();
+
+    @Override
+    protected void initTracing(ServletContextHandler context) {


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.

rnorth · 2018-10-17T12:14:39Z

...g-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java

+         *                               Default is false.
+         * @return builder
+         */
+        public Builder withJoinExistingActiveSpan(boolean joinExistingActiveSpan) {


Are you happy with the naming of this? I couldn't think of much better, but perhaps it could be improved.

rnorth · 2018-10-17T12:16:11Z

opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingFilter.java

@@ -64,6 +67,7 @@ public void filter(ContainerRequestContext requestContext) {
        if (tracer != null) {

            Tracer.SpanBuilder spanBuilder = tracer.buildSpan(operationNameProvider.operationName(requestContext))
+                    .ignoreActiveSpan()


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.

…ured. The ensures that the damage caused by accidental thread local leakage between requests is minimized Fixes opentracing-contrib#109

rnorth · 2018-10-22T08:40:08Z

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

rnorth · 2018-10-22T08:40:55Z

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

rnorth · 2018-10-22T08:42:11Z

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

rnorth · 2018-10-23T09:23:59Z

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

sjoerdtalsma · 2018-10-29T19:56:14Z

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 opentracing/opentracing-java#313

rnorth · 2018-10-30T10:28:08Z

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

Thanks, It looks good just a couple of minor comments.

pavolloffay · 2018-10-30T17:01:56Z

...g-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java

@@ -197,6 +201,18 @@ public Builder withSkipPattern(String skipPattern) {
            return this;
        }

+        /**
+         * @param joinExistingActiveSpan If true, any active span on the current scope will be used as the parent span.


on the current thread. This can be used when combining with servlet instrumentation.

pavolloffay · 2018-10-30T17:02:07Z

...g-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java

@@ -197,6 +201,18 @@ public Builder withSkipPattern(String skipPattern) {
            return this;
        }

+        /**
+         * @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


If false parent span will be extracted from HTTP headers.

pavolloffay · 2018-10-30T17:08:43Z

...c/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java

+        if (shouldUseParentSpan()) {
+            Assert.assertEquals(preceding.context().spanId(), original.parentId());
+        } else {
+            Assert.assertNotEquals(preceding.context().spanId(), original.parentId());


You can equal to 0 which means no parent

pavolloffay · 2018-10-30T17:12:40Z

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 · 2018-10-30T18:11:59Z

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 · 2018-10-31T10:02:21Z

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

rnorth · 2018-10-31T11:49:43Z

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 · 2018-10-31T12:54:57Z

thanks @rnorth

rnorth commented Oct 17, 2018

View reviewed changes

rnorth force-pushed the configurable-active-span-joining branch from 1a07cee to 40ab1aa Compare October 22, 2018 08:34

Make active span be ignored by default; used as parent span if config…

6559b80

…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 October 22, 2018 08:36

rnorth closed this Oct 22, 2018

rnorth reopened this Oct 22, 2018

empty commit to refresh PR

049d2e9

pavolloffay reviewed Oct 30, 2018

View reviewed changes

Changes based on review comments from @pavolloffay

f5ef880

pavolloffay approved these changes Oct 31, 2018

View reviewed changes

pavolloffay merged commit 1228471 into opentracing-contrib:master Oct 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rnorth commented Oct 17, 2018

rnorth Oct 17, 2018 •

edited

Loading

rnorth Oct 17, 2018

rnorth Oct 17, 2018

rnorth Oct 17, 2018

rnorth commented Oct 22, 2018

rnorth commented Oct 22, 2018

rnorth commented Oct 22, 2018

rnorth commented Oct 23, 2018

sjoerdtalsma commented Oct 29, 2018

rnorth commented Oct 30, 2018

pavolloffay left a comment

pavolloffay Oct 30, 2018

pavolloffay Oct 30, 2018

pavolloffay Oct 30, 2018

pavolloffay commented Oct 30, 2018

sjoerdtalsma commented Oct 30, 2018

pavolloffay commented Oct 31, 2018

rnorth commented Oct 31, 2018

pavolloffay commented Oct 31, 2018

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

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

Conversation

rnorth commented Oct 17, 2018

rnorth Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

rnorth Oct 17, 2018

Choose a reason for hiding this comment

rnorth Oct 17, 2018

Choose a reason for hiding this comment

rnorth Oct 17, 2018

Choose a reason for hiding this comment

rnorth commented Oct 22, 2018

rnorth commented Oct 22, 2018

rnorth commented Oct 22, 2018

rnorth commented Oct 23, 2018

sjoerdtalsma commented Oct 29, 2018

rnorth commented Oct 30, 2018

pavolloffay left a comment

Choose a reason for hiding this comment

pavolloffay Oct 30, 2018

Choose a reason for hiding this comment

pavolloffay Oct 30, 2018

Choose a reason for hiding this comment

pavolloffay Oct 30, 2018

Choose a reason for hiding this comment

pavolloffay commented Oct 30, 2018

sjoerdtalsma commented Oct 30, 2018

pavolloffay commented Oct 31, 2018

rnorth commented Oct 31, 2018

pavolloffay commented Oct 31, 2018

rnorth Oct 17, 2018 •

edited

Loading