-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CDI context propagation improvements for the reactive stack #27443
Conversation
@Sanne would you be able to benchmark this PR (with CP disabled) to get some actual numbers? |
3b18c96
to
6893a45
Compare
This comment has been minimized.
This comment has been minimized.
@radcortez @brunobat Hey guys, the @cescoffier may also add some relevant info because he fixed some OTel reactive problem recently. |
I didn't fix anything; I've found some code that would need to be changed to support this PR. OTel is duplicating duplicated context to create a new span. With this PR, you cannot do that anymore. We need to find another way, like marking the context or something like this. |
Eh, yes that commit is actually pretty old :D |
Also, please rebase onto |
6893a45
to
297cb93
Compare
I believe we are missing a span like this, for the 2nd client call, with QueryParam Goku: |
@brunobat yes, because the initial code was switching to another duplicated context without going back to it after the operation. So either we need to implement this second switch or we find another way to trigger the creation of the span. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@cescoffier ^ problem with formatting... |
3edf30d
to
7382676
Compare
@mkouba fixed. I added the RR and a GrapQL tests as discussed. |
This comment has been minimized.
This comment has been minimized.
I've changed the registration of the OpenTelemetryClientFilter only to be applied in the RESTEasy Classic Client. This avoids the double spans for the Reactive Client, which also uses the Vert.x Tracer. |
I would also like to add some additional tests on the OTel side. |
...try/deployment/src/main/java/io/quarkus/opentelemetry/deployment/OpenTelemetryProcessor.java
Outdated
Show resolved
Hide resolved
...main/java/io/quarkus/opentelemetry/runtime/tracing/restclient/OpenTelemetryClientFilter.java
Outdated
Show resolved
Hide resolved
...me/src/main/java/io/quarkus/opentelemetry/runtime/tracing/vertx/InstrumenterVertxTracer.java
Show resolved
Hide resolved
@radcortez what about testing that the span "tree" is correct (what I did manually yesterday)? The purpose would be to detect changes in the structure when we add more OTel support in Mutiny (because typically, for mutiny, the tree is slightly different with the last span having two parents). |
1f7d1d2
to
00ce709
Compare
...main/java/io/quarkus/opentelemetry/runtime/tracing/restclient/OpenTelemetryClientFilter.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
00ce709
to
6bd931c
Compare
== ArC Introduce the VertxCurrentContextFactory so that the Vert.x duplicated context can be used to store the "current context" for normal scopes. == resteasy-reactive Don't clear our CDI current request when suspending. This used to be done in order to prevent subsequent requests running on the same thread as a suspended request from accessing the former's data. With the advent of DuplicatedContext backed storage, there is no longer any chance of mixing data so there is no need to clear it out. Furthermore, by not clearing out current request, code that accesses the request scoped CurrentVertxRequest that is executed while the request is suspended, can now work even if context propagation is not in play. == Tests Add leak detection tests for resteasy reactive, graphql and reactive rest client. Also improve the OpenTelemetry reactive tests. == OpenTelemetry Only register the OpenTelemetryClientFilter for RESTEasy Client Classic. Use Capabilities to determine when to register the OpenTelemetryClientFilter. Co-authored-by: Georgios Andrianakis <[email protected]> Co-authored-by: Clement Escoffier <[email protected]> Co-authored-by: Roberto Cortez <[email protected]> Co-authored-by: brunobat <[email protected]> Co-authored-by: Martin Kouba <[email protected]>
55877b0
to
2b64b81
Compare
Triggered adhoc GitHub Action Daily run to check possible side-effects and the run was green. |
Fixes failing CI tests. Somehow being related to this change here quarkusio/quarkus#27443. I don't think that our use of `CompleteableFuture` is wrong, it's what has been suggested a while back to chose over our own drivers reactive types and we am not gonna change that. The regex is now more lenient. FWIW if anything should change than Quarkus and the CI system should make sure it works both the same way in the CI in *this* project here and the overall integration project running on the whole of Quarkivere.
This PR implements a new way to store the ArC request scope. Instead of storing that object in a
ThreadLocal
(which is problematic in an event loop-based architecture), the request scope is stored in the Vert.x duplicated context which is used for the whole reactive/async processing. If there is no duplicated context, it falls back to a thread-local.This PR makes accessing request-scope beans a lot simpler in reactive applications. The request scope is available as soon as the continuations are executed on the same duplicated context. We aware that if this access happens after the termination of the scope, an error is reported (as it's clearly misuing the concepts).
ArC
Introduce the
VertxCurrentContextFactory
so that the Vert.x duplicated context can be used to store the "current context" for normal scopes. If we're not on the Vert.x duplicated context we fallback to theThreadLocal
(orFastThreadLocal
respectively).resteasy-reactive
Don't clear our CDI current request when suspending.
This was done to prevent subsequent requests running on the same thread as a suspended request from accessing the former's data. With the advent of DuplicatedContext-backed storage, there is no longer any chance of mixing data, so there is no need to clear it out.
Furthermore, by not clearing out current requests, the code that accesses the request scoped
CurrentVertxRequest
that is executed while the request is suspended can now work even if context propagation is not in play.
Tests
Add leak detection tests for resteasy reactive, graphql, and reactive rest client. Also, improve the OpenTelemetry reactive tests.
OpenTelemetry
Only register the OpenTelemetryClientFilter for RESTEasy Client Classic. Use Capabilities to determine when to register the OpenTelemetryClientFilter.