vvivekiyer commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r903060215
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -682,7 +697,8 @@ private void logBrokerResponse(long requestId, String
query, RequestContext requ
brokerResponse.getOfflineThreadCpuTimeNs(),
brokerResponse.getOfflineSystemActivitiesCpuTimeNs(),
brokerResponse.getOfflineResponseSerializationCpuTimeNs(),
brokerResponse.getRealtimeTotalCpuTimeNs(),
brokerResponse.getRealtimeThreadCpuTimeNs(),
brokerResponse.getRealtimeSystemActivitiesCpuTimeNs(),
- brokerResponse.getRealtimeResponseSerializationCpuTimeNs(),
StringUtils.substring(query, 0, _queryLogLength));
+ brokerResponse.getRealtimeResponseSerializationCpuTimeNs(),
+ remoteIps == null ? "" : StringUtils.join(remoteIps, ";"),
StringUtils.substring(query, 0, _queryLogLength));
Review Comment:
Can you please post an output of how this log looks like?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -670,7 +684,8 @@ private void logBrokerResponse(long requestId, String
query, RequestContext requ
+
"segments(queried/processed/matched/consuming/unavailable):{}/{}/{}/{}/{},consumingFreshnessTimeMs={},"
+
"servers={}/{},groupLimitReached={},brokerReduceTimeMs={},exceptions={},serverStats={},"
+
"offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):{}/{}/{}/{},"
- +
"realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):{}/{}/{}/{},query={}",
requestId, tableName,
+ +
"realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):{}/{}/{}/{},remoteIps={}"
Review Comment:
Should we call this clientIP instead?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -586,8 +593,14 @@ private BrokerResponseNative handleRequest(long requestId,
String query, JsonNod
requestContext.setQueryProcessingTime(totalTimeMs);
augmentStatistics(requestContext, brokerResponse);
+ // Extract source info from incoming request
+ Collection<String> remoteIps = null;
+ if (requesterIdentity != null) {
+ remoteIps = ((HttpRequesterIdentity)
requesterIdentity).getHttpHeaders().get("X-Forwarded-For");
Review Comment:
Typically reverse proxies are used. So X-Forwarded-For will be populated.
But do we want to populate this field with the clientIP if the request is not
forwarded as well?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -453,8 +455,13 @@ private BrokerResponseNative handleRequest(long requestId,
String query, JsonNod
// Send empty response since we don't need to evaluate either offline or
realtime request.
BrokerResponseNative brokerResponse = BrokerResponseNative.empty();
+ // Extract source info from incoming request
+ Collection<String> remoteIps = null;
Review Comment:
nit: code is repeated. Also, we do this processing even if the log is not
printed (because of rateLimiter). May be just pass in requesterIdentity to
logBrokerResponse() and perform this computation only if we are going to log
the metrics?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
