Add helper methods for manual spans in mutiny pipelines #45478
Conversation
|
Thanks for your pull request! Your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
|
/cc @brunobat (opentelemetry), @radcortez (opentelemetry) |
arn-ivu
changed the title
...e/src/test/java/io/quarkus/opentelemetry/runtime/tracing/mutiny/MutinyTracingHelperTest.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
|
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I need to read the motivation behind the A helper method like this doesn't need to be redundant, but I don't see why it wouldn't be implemented the same way. As for what has been done in |
|
@ozangunalp My usecase looks roughly like this it might be possible to solve it like this however, I have 2 problems with that
|
|
@arn-ivu ok, I think that can justify the helper method. public static <T> Uni<T> wrapWithSpan(final Tracer tracer,
final Optional<Context> parentContext,
final String spanName, final Uni<T> pipeline) {
return runOnDuplicatedContext(Uni.createFrom().deferred(() -> {
final SpanBuilder spanBuilder = tracer.spanBuilder(spanName);
if (parentContext.isPresent()) {
spanBuilder.setParent(parentContext.get());
} else {
spanBuilder.setNoParent();
}
final Span span = spanBuilder.startSpan();
final Scope scope = span.makeCurrent();
return pipeline.onTermination()
.invoke(new Functions.TriConsumer<T, Throwable, Boolean>() {
@Override
public void accept(T o, Throwable throwable, Boolean isCancelled) {
if (isCancelled) {
span.recordException(new CancellationException());
} else if (throwable != null) {
span.recordException(throwable);
}
span.end();
scope.close();
}
});
}));
}Note that lambda described in deferred runs on subscription time Which is much closer to what we do in wdyt? cf @brunobat |
|
It took me a little to understand the suggested code as I am not that familiar with mutiny, but it now looks good to me. I have updated the code accordingly. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Ok for me except the formatting issue, @brunobat would you also take a look ? |
| span.recordException(throwable); | ||
| } | ||
| span.end(); | ||
| scope.close(); |
There was a problem hiding this comment.
I think the scope close should be on a finally block. It's a good practice.
See
|
I think it's good. Just left a comment about a minor thing. |
arn-ivu
force-pushed
the
mutiny-tracing-helper
branch
from
e2eaf91 to
26eeeee
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
To fix the imports, you just need to run |
In a traditional blocking and synchronous framework the opentelemetry context is attached to ThreadLocal. In reactive programming, where multiple processings share the same event-loop thread, one has to use Vert.x duplicated contexts instead (see https://quarkus.io/guides/duplicated-context). wrapWithSpan ensures that the pipeline is executed on a duplicated context (If the current context already is duplicated, it will stay the same. Therefore, nested calls to wrapWithSpan will all run on the same vert.x context). inspired by Jan Peremsky (https://github.com/jan-peremsky/quarkus-reactive-otel/blob/c74043d388ec4df155f466f1d6938931c3389b70/src/main/java/com/fng/ewallet/pex/Tracer.java) and edeandrea (https://github.com/quarkusio/quarkus-super-heroes/blob/main/event-statistics/src/main/java/io/quarkus/sample/superheroes/statistics/listener/SuperStats.java) suggestions from review I will squash with the previous commit once everything is approved suggestion from reviewer todo: squash when everything is done -> adapt commit message. In this solution span and scope are local variables. no need for a stack. unnecessary line removed fix test to run on different contexts again cleanup
arn-ivu
force-pushed
the
mutiny-tracing-helper
branch
from
26eeeee to
1d9029d
Compare
Status for workflow
|
Status for workflow
|
|
Hey there, I have seen these new tests fail on CI, for instance:
In this case, we have more spans than expected and thus the test fails. I also experienced some local failures when I was working on the OpenTelemetry module. Could we make sure these tests are made more reliable? Thanks! |
|
Will take a look at that test |
|
@brunobat Thanks! ping me if I can help |
|
Let's see if #45653 fixes the issue. |
Fix #44411
In a traditional blocking and synchronous framework the opentelemetry context is attached to ThreadLocal. In reactive programming, where multiple processings share the same event-loop thread, one has to use Vert.x duplicated contexts instead (see https://quarkus.io/guides/duplicated-context). wrapWithSpan ensures that the pipeline is executed on a duplicated context (If the current context already is duplicated, it will stay the same. Therefore, nested calls to wrapWithSpan will all run on the same vert.x context).
Another difficulty of mutiny pipelines is, that usually only one parameter flows through the pipeline. In oder to end spans and to close the current scope at the end of the pipeline, span and scope must be stored in the mutiny-context. They are stored in a stack datastructure so that multiple nested spans/scopes are closed in the correct order.
inspired by Jan Peremsky (https://github.com/jan-peremsky/quarkus-reactive-otel/blob/c74043d388ec4df155f466f1d6938931c3389b70/src/main/java/com/fng/ewallet/pex/Tracer.java) and edeandrea (https://github.com/quarkusio/quarkus-super-heroes/blob/main/event-statistics/src/main/java/io/quarkus/sample/superheroes/statistics/listener/SuperStats.java)