Update to use the OT Java 0.31.0-RC1 API #27
Conversation
| @@ -217,7 +217,9 @@ public void onStartAsync(AsyncEvent event) throws IOException { | |||
| } | |||
| }); | |||
| } | |||
| span.deactivate(); | |||
| if (!httpRequest.isAsyncStarted()) { | |||
There was a problem hiding this comment.
Originally tried using this in the startActive, but it doesn't return an appropriate value at that point - so the issue is that currently this parent scope is not closed. So this needs to be reported as feedback on the OT java API RC. A possible approach may be to have an overloaded close that allows the finishSpan to be specified at the close - for cases where the decision is unknown when the scope is started.
There was a problem hiding this comment.
Just to be clear - does this currently work by checking httpRequest.isAsyncStarted() and then closing the span?
There was a problem hiding this comment.
We should close the scope for sync and async. The thread no longer belongs to the request.
I suggest using finishOnClose = false when starting the span. Then in async callbacks use finishOnClose = true.
Here is the patch https://pastebin.com/JT67yUYH
There was a problem hiding this comment.
Updated to use the approach discussed - so when async/sync not know when the Scope is started - it starts with 'finishOnClose=false' and then explicitly closes the span associated with the Scope if turns out to be synchronous.
| @@ -4,7 +4,7 @@ | |||
|
|
|||
| <groupId>io.opentracing.contrib</groupId> | |||
| <artifactId>opentracing-web-servlet-filter-parent</artifactId> | |||
| <version>0.0.10-SNAPSHOT</version> | |||
| <version>0.1.0-RC1-SNAPSHOT</version> | |||
There was a problem hiding this comment.
Thought it might be worth bumping the minor version as there are breaking changes.
There was a problem hiding this comment.
Looks good, perhaps we could merge it to v0.1.0-RC1 branch. (We have to include the modified publis.sh)
There was a problem hiding this comment.
Is it just a straight copy of the publish.sh from opentracing-java?
There was a problem hiding this comment.
I will raise a PR against the master, the current version of the script has to be modified.
There was a problem hiding this comment.
Ah ok - although I've just added it to this PR.
There was a problem hiding this comment.
opentracing/opentracing-java#198
This change is missing in the script. Please add it once it's merged. Or we can add it as a separate PR to the master.
There was a problem hiding this comment.
This can also be left as it is. The version will be changed by release process.
… span associated with scope
| } | ||
| throw ex; | ||
| } finally { | ||
| if (httpRequest.isAsyncStarted()) { | ||
| final ActiveSpan.Continuation cont = span.capture(); | ||
| final Span cont = scope.span(); |
There was a problem hiding this comment.
Do we need to define it here? What about passing the span directly to tracer.scopeManager().activate(cont)
There was a problem hiding this comment.
The scope could be closed - which makes accessing the span potentially unpredicatable - I don't know if it is defined whether the span remains accessible after the scope is closed?
There was a problem hiding this comment.
the scope could be closed
Could you please explain it more? This scope is accessible only from this class.
There was a problem hiding this comment.
I guess there is nothing in the method that says Span is not accessible after close so will change.
travis/publish.sh
Outdated
| # and we want that branch to be master or v0.0.0. which has been checked before. | ||
| # But we also want to make sure that we build and release exactly the tagged version, | ||
| # so we verify that the remote branch is where our tag is. | ||
| git checkout -B v`release_version | sed 's/-RC[[:digit:]]\+//'` |
There was a problem hiding this comment.
This has to be changed.
It should distinguish between master and version branch. Please update it with ot-java repo.
There was a problem hiding this comment.
Then it can be merged/released
There was a problem hiding this comment.
Assume you meant to just copy it from ot-java repo - if so, done 👍
No description provided.