adutra commented on code in PR #2914: URL: https://github.com/apache/polaris/pull/2914#discussion_r2468707407
########## runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java: ########## @@ -18,60 +18,38 @@ */ package org.apache.polaris.service.tracing; -import io.smallrye.mutiny.Uni; import jakarta.inject.Inject; import jakarta.ws.rs.container.ContainerRequestContext; import jakarta.ws.rs.container.ContainerResponseContext; -import jakarta.ws.rs.core.MediaType; -import jakarta.ws.rs.core.Response; -import org.apache.iceberg.rest.responses.ErrorResponse; import org.apache.polaris.service.config.FilterPriorities; -import org.apache.polaris.service.logging.LoggingConfiguration; import org.jboss.resteasy.reactive.server.ServerRequestFilter; import org.jboss.resteasy.reactive.server.ServerResponseFilter; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +/** + * Filter that handles request IDs for tracing. + * + * <p>See <a + * href="https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/observability/tracing";>Envoy + * Tracing</a> + */ public class RequestIdFilter { public static final String REQUEST_ID_KEY = "requestId"; - private static final Logger LOGGER = LoggerFactory.getLogger(RequestIdFilter.class); - - @Inject LoggingConfiguration loggingConfiguration; - @Inject RequestIdGenerator requestIdGenerator; + @Inject TracingConfiguration tracingConfiguration; @ServerRequestFilter(preMatching = true, priority = FilterPriorities.REQUEST_ID_FILTER) - public Uni<Response> assignRequestId(ContainerRequestContext rc) { - var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName()); - return (requestId != null - ? Uni.createFrom().item(requestId) - : requestIdGenerator.generateRequestId(rc)) - .onItem() - .invoke(id -> rc.setProperty(REQUEST_ID_KEY, id)) - .onItemOrFailure() - .transform((id, error) -> error == null ? null : errorResponse(error)); + public void extractRequestId(ContainerRequestContext rc) { + var requestId = rc.getHeaderString(tracingConfiguration.requestId().headerName()); + rc.setProperty(REQUEST_ID_KEY, requestId); } @ServerResponseFilter public void addResponseHeader( ContainerRequestContext request, ContainerResponseContext response) { String requestId = (String) request.getProperty(REQUEST_ID_KEY); - if (requestId != null) { // can be null if request ID generation fails - response.getHeaders().add(loggingConfiguration.requestIdHeaderName(), requestId); + if (requestId != null) { + response.getHeaders().add(tracingConfiguration.requestId().headerName(), requestId); Review Comment: I did some archelogy prior to working on this, and my conclusion is that we _probably_ want a response header. First, Dropwizard has a request ID fitler: https://github.com/dropwizard/dropwizard/blob/3833f0e1d9fb8cae256fe09379733b8d651f8b87/dropwizard-jersey/src/main/java/io/dropwizard/jersey/filter/RequestIdFilter.java#L42-L49 Note how this filter creates random request IDs (!!!) _and_ sets a response header. I don't know for sure if this filter is enabled by default. I think it is, because `dropwizard-core` depends on `dropwizard-jersey`. It exists since 2017. OTOH, Polaris-Dropwizard only had this small line alluding to request IDs: https://github.com/apache/polaris/blob/4b18ec065ff16f74b11bc85fdc6ea9036eca7274/dropwizard/service/src/main/java/org/apache/polaris/service/dropwizard/PolarisApplication.java#L516 But you'll notice that the header is now `request_id` 🤷 . I wonder what the intent was here. Furthermore the only other usage of request IDs in Polaris-Dropwizard seems to be in the log pattern: https://github.com/apache/polaris/blob/4b18ec065ff16f74b11bc85fdc6ea9036eca7274/polaris-server.yml#L150 Notice the mysterious `{rid}` MDC context, that seems to mean "request ID". This is surprising. I would expect `request_id` instead, since that's the MDC variable name. All of this to say: 1. I have no idea how this could be working 🤣 2. I think it doesn't hurt to echo the request ID in the response header. WDYT? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
