Bump OT to 0.30.0 #36
Conversation
| @@ -59,7 +57,8 @@ public void filter(ContainerRequestContext requestContext) throws IOException { | |||
| spanBuilder.asChildOf(extractedSpanContext); | |||
| } | |||
|
|
|||
| Span span = spanBuilder.start(); | |||
| Span span = spanBuilder.startManual(); | |||
| tracer.makeActive(span); | |||
There was a problem hiding this comment.
This is ugly and the reason for this is that in async requests the thread in response filter is different and active span cannot be deactivated/finished: https://github.com/opentracing/opentracing-java/blob/master/opentracing-util/src/main/java/io/opentracing/util/ThreadLocalActiveSpan.java#L50.
The most ideal solution would be to instrument ExecutorService/or something like onAsync but I could not find a way how to do it in jax-rs.
Other options:
- do not activate at all (not wanted)
- create "manual" span and activate explicitly.
- activate only for sync requests. (this is Brave doing https://github.com/pavolloffay/brave/blob/629a7da8d8e650124918b0e613b68bedd39cef34/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/TracingContainerFilter.java#L74-L74)
There was a problem hiding this comment.
tracer.makeActive(span); - will make span active forever? (if thread is not destroyed because it is taken from thread pool and returned back). So you also need to deactivate span explicitly.
There was a problem hiding this comment.
@malafeev exactly, I think 3. is the most consistent solution. If we cannot deactivate span then do not activate. I'm wondering what other people think about this.
There was a problem hiding this comment.
Have you tried using a continuation?
There was a problem hiding this comment.
What do you mean e.g. could you outline possible solution? What should be done in request and response filters.
There was a problem hiding this comment.
One possible approach would be to replace in the request filter requestContext.setProperty(SPAN_PROP_ID, new SpanWrapper(span)); by a request context property that stores the Continuation returned from span.capture() - and on a following line do span.deactivate().
Then in the response filter retrieve the Continuation and activate it to retrieve the active span which can then be deactivated at the end of the method.
There was a problem hiding this comment.
@objectiser if span is deactivated in request filter then during processing of request (before response filter) span will not be picked up as parent span. In such case there is no sense to activate/deactivate span.
For example:
- Client send request
- Server Endpoint send another REST request. Here we should get active span from request filter to use as parent for new REST request.
- Server send response
There was a problem hiding this comment.
One possible approach would be to replace in the request filter requestContext.setProperty(SPAN_PROP_ID, new SpanWrapper(span)); by a request context property that stores the Continuation returned from span.capture() - and on a following line do span.deactivate().
Then in the response filter retrieve the Continuation and activate it to retrieve the active span which can then be deactivated at the end of the method.
As @malafeev said this does not make sense, we could simply use Span and just finish it.
There was a problem hiding this comment.
Yeah was forgetting about the child spans :)
There was a problem hiding this comment.
@pavolloffay Would be worth creating an issue on opentracing-java to see if a general solution can be found.
| protected Client getClient() { | ||
| return new ResteasyClientBuilder() | ||
| /** | ||
| * To awoid RESTEASY004655 "connection is still allocated" in {@link #testAsyncMultipleRequests()} |
|
PR updated. Based on discussion with @jpkrohling, span is activated only in sync requests, because we should make sure that the span can be properly deactivated. |
|
@jpkrohling @objectiser could you please re-review? Or is it good to go? |
README.md
Outdated
| ### Async | ||
| Async requests are executed in a different thread than when the client has been invoked, therefore | ||
| spans representing client requests are not connected to appropriate parent. To fix this JAX-RS client | ||
| has to use OpenTracing-aware `ExecutorService`. |
There was a problem hiding this comment.
Would be good to provide a reference to java-concurrent.
There was a problem hiding this comment.
For example, TracedExecutorService is used in a code snippet below but there is no clue for user where to find it.
There was a problem hiding this comment.
Link added, actually that dependency is pulled by default so users would find it.
|
@jpkrohling @objectiser thanks |
No description provided.