dachaostripe commented on issue #9545: URL: https://github.com/apache/pinot/issues/9545#issuecomment-1271224575
you mean like `queryRequest.getBrokerId()` used at [QueryScheduler](https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/QueryScheduler.java#L257)? which presumably is originated from here at [BaseBrokerRequestHandler](https://github.com/apache/pinot/blob/master/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java#L120)? However, the same brokerId doesn't seem being used for logging within the broker, and broken doesn't seem to even make an info/summary log in normal case, assuming this is [the end of request cycle](https://github.com/apache/pinot/blob/master/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java#L712-L716). While it may be possible to make log tradable among components with the (requestId, brokerId) combo, with substantial work it seems, it's still an inefficient and much less convenient approach compared to if we can make them traceable with just a single requestId, if we can just make this requestId globally unique, either as an UUID, or like the [globalQueryId](https://github.com/apache/pinot/blob/0fa93971875fadbca17e5577fc45a9434d395329/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java#L1649) you're already making here: `_brokerId + "_" + requestId` By skimming through the 2 classes I linked above, I found the general pattern of logging is like `LOGGER.error("Some message, requestId={}", requestId)`, which in my opinion is very tedious, inconsistent, and unreliable, that not every log is following the pattern and those untagged logs becomes practically unsearchable in a high traffic production environment. I'd suggest a more reliable approach of adding a request filter/valve in front of the handler, to put requestId into Log4j's threadContext, like this ``` beforeHandler(Request req) { val requestId = Optional.of(req.getHeader("x-pinot-requestId")).getOrElse(UUID.newId()); MDC.put("requestId", requestId); } afterHandler(Response resp) { val requestId = MDC.remove("requestId"); resp.setHeader("x-pinot-requestId", requestId); } ``` Along with proper config in log4j2.xml like ``` <PatternLayout pattern="%d{HH:mm:ss.SSS} {requestId} %p [%c{1}] [%t] %m%n"/> ``` Then all logs made with something like `LOGGER.error("Some message")` will automatically look like this in logs ``` [DateTime] REQUEST_ID processId ... Some message. ``` This will automatically cover all log messages made within a request cycle and make them easily searchable. In fact, it's already doing something similar [here](https://github.com/apache/pinot/blob/0fa93971875fadbca17e5577fc45a9434d395329/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java#L263-L265) with the requestContext, we just need to extend this to log context as well. -- 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: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
