[4.0] Migrate the OpenTelemetry extension to Vert.x 5#53755
[4.0] Migrate the OpenTelemetry extension to Vert.x 5#53755cescoffier wants to merge 2 commits intoquarkusio:vertx-5-migrationfrom
Conversation
- Rework the context storage - Change how gRPC tracing is working as we do not have gRPC java anymore (so, we have both the gRPC and HTTP spans, all linked together)
|
/cc @brunobat (opentelemetry), @radcortez (opentelemetry) |
| import io.quarkus.test.QuarkusExtensionTest; | ||
| import io.restassured.RestAssured; | ||
|
|
||
| @Disabled |
There was a problem hiding this comment.
Note: re-enable once we have an smallrye-graphql Vert.x. 5 release upstream
| if (route != null) { | ||
| requestMetric.getContext().ifPresent(context -> context.putLocal("VertxRoute", route)); | ||
| requestMetric.getContext() | ||
| .ifPresent(context -> context.getLocal(VertxContext.DATA_MAP_LOCAL).put("VertxRoute", route)); |
There was a problem hiding this comment.
Just in case, add ConcurrentHashMap::new
There was a problem hiding this comment.
Weird, I was pretty sure to use the other API. Let me fix it.
|
|
||
| private static final Logger log = Logger.getLogger(QuarkusContextStorage.class); | ||
| private static final String OTEL_CONTEXT = QuarkusContextStorage.class.getName() + ".otelContext"; | ||
| private static final ContextLocal<Context> CONTEXT_OVERRIDE = ContextLocal.registerLocal(Context.class); |
There was a problem hiding this comment.
Do you have a VertxServiceProvider to ensure it's loaded on time?
There was a problem hiding this comment.
Hum, no, need to add it.
| this.client = vertx.httpClientBuilder() | ||
| .with(httpClientOptions) | ||
| .with(httpClientOptions.getPoolOptions()) | ||
| .with(new PoolOptions()) // TODO Do we want to customize pool options? |
There was a problem hiding this comment.
Are the default values equivalent to Vert.x 4?
There was a problem hiding this comment.
Then we shouldn't customize the PoolOptions
|
|
||
| private static final Logger log = Logger.getLogger(QuarkusContextStorage.class); | ||
| private static final String OTEL_CONTEXT = QuarkusContextStorage.class.getName() + ".otelContext"; | ||
| private static final ContextLocal<Context> CONTEXT_OVERRIDE = ContextLocal.registerLocal(Context.class); |
There was a problem hiding this comment.
What's the difference between ContextLocal and VertxContext.localContextData(vertxContext) ?
There was a problem hiding this comment.
ContextLocal is the Vert.x 5 way to define context storage, in Vert.x 4 this used to be a big map for getLocal / putLocal access. Now we get a more fine-grained storage where each context storage can define what happens on duplication, etc.
VertxContext provides helpers in smallrye-common, especially the parent-child duplicated contexts we use in Quarkus.
There was a problem hiding this comment.
Then... Will ContextLocal and VertxContext.localContextData(vertxContext) get the same data?
| public void onClose(final Status status, final Metadata trailers) { | ||
| instrumenter.end(spanContext, grpcRequest, status, status.getCause()); | ||
| try (scope) { | ||
| QuarkusContextStorage.clearContextOverride(Vertx.currentContext()); |
There was a problem hiding this comment.
I don't understand this part.
Why do we need to clear the context in this particular case?
There was a problem hiding this comment.
Because SR Context propagation was in the way and clear things that should not be cleared. So, I had to find something different.
There was a problem hiding this comment.
I would like to find a way to avoid this - but, ran out of options/time.
There was a problem hiding this comment.
Ok, Can you just add a comment in the method to when it should be used?
| ConcurrentMap<String, Object> local = VertxContext.localContextData(vertxContext); | ||
| if (containsIgnoredKey(ignoredKeys, local)) { | ||
| // Duplicate the context, copy the data, remove the request context | ||
| ConcurrentHashMap<String, Object> mdcData = vertxContext.getLocal(VertxMDC.MDC_LOCAL); |
There was a problem hiding this comment.
Why only with the VertxMDC.MDC_LOCAL?
jponge
changed the title
|
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. |
3 participants
In draft, as we are waiting for SmallRye GraphQL.