Provide the ability to log MDC data in access_log #27006
Conversation
pilhuhn
changed the title
|
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
|
This comment has been minimized.
This comment has been minimized.
| @Override | ||
| public String readAttribute(RoutingContext exchange) { | ||
| Map<String, Object> mdcMap = exchange.vertx().getOrCreateContext().getLocal("io.quarkus.vertx.core.runtime.VertxMDC"); | ||
| return (String) mdcMap.getOrDefault(dataKey, null); |
There was a problem hiding this comment.
They will be there only if the open telemetry extension is used... I wonder if this can be implemented on it.
The tests will never pass without it.
See: io.quarkus.opentelemetry.runtime.tracing.binders.vertx.EventBusInstrumenterVertxTracer
There was a problem hiding this comment.
I know this is tricky. I was thinking this could go into the open-telemetry extension, but then [ExchangeAttributeBuilder](https://github.com/quarkusio/quarkus/pull/27006#diff-287774a4f1c3b110b81bac31ef5eafa4372d24c820feb95563288be11238b421) can't list it and I don't think there is something like a ServiceLoader mechanism available. Perhaps @stuartwdouglas knows?
There was a problem hiding this comment.
The service loader is available but probably not needed.
We can try to bind this ExchangeAttribute like we do for the vert.x hooks. See:
There was a problem hiding this comment.
I think you should use VertxMDC.INSTANCE directly if you need to pass a Vert.x context , as it was done here https://github.com/quarkusio/quarkus/blob/main/extensions/opentelemetry/opentelemetry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/OpenTelemetryUtil.java#L99 or call MDC.get(dataKey)); since VertxMDC will be always there, and you can log everything in the MDC not only Otel stuff
There was a problem hiding this comment.
The thing is that only OTEL might be adding stuff to the MDC now, but the Vertx MDC implementation is in Vertx, and not in the OTEL extension, so I would really make this agnostic, and use the Vertx MDC instead of attaching it to OTEL
There was a problem hiding this comment.
I think you should use
VertxMDC.INSTANCEdirectly
Great suggestion. I wasn't aware of it, but that makes life easier for everyone.
There was a problem hiding this comment.
The thing is that only OTEL might be adding stuff to the MDC now, but the Vertx MDC implementation is in Vertx, and not in the OTEL extension, so I would really make this agnostic, and use the Vertx MDC instead of attaching it to OTEL
You are right. I am renaming this and also update documentation.
There was a problem hiding this comment.
The weird thing will be to see empty traceId and spanIds when the OTel extension is not used
There was a problem hiding this comment.
But that is also true for the console.log.format pictured here:
https://quarkus.io/guides/opentelemetry#create-the-configuration
There was a problem hiding this comment.
yeah... it can probably be improved
|
@pilhuhn can you please add a test? |
pilhuhn
force-pushed
the
access_log_mdc_data
branch
from
8940bc9 to
979883d
Compare
A config entry in application.properties like:
quarkus.http.access-log.pattern="%h %l %u %t \"%r\" %s %b traceId=%{X,traceId}"would end up in
2022-07-28 17:46:56,512 INFO [access_log] (executor-thread-2) "127.0.0.1 - hrupp 28/Jul/2022:17:46:56 +0200 "GET /api/policies/v1.0/policies HTTP/1.1" 404 - traceId=797e67544634cff6b25a7d6ab429e67e"This is similar to #26339
Unfortunately the
%{X,..}syntax in access log patterns is different from the%X{..}syntax for console logs.cc @brunobat