You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/06/16 16:26:14 UTC
[GitHub] [pinot] abhs50 opened a new pull request, #8907: Log Request header for Broker
abhs50 opened a new pull request, #8907:
URL: https://github.com/apache/pinot/pull/8907
### Background
Add request header to broker logs
See #8869 for more details.
### Release Note
This PR introduces passing a new parameter **requesterIdentity** to broker logs, so we can track the source info of queries being sent to the broker
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mcvsubbu commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r927835432
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
Review Comment:
```suggestion
String clientIp = "0";
```
I suggest not leaving it as null, so the parsing scripts will still work fine. many expect key=value pair. thanks.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mcvsubbu commented on pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on PR #8907:
URL: https://github.com/apache/pinot/pull/8907#issuecomment-1158137791
Can we make this optional (configurable on the broker config)? You can keep the default ON. In our company, requests come via an endpoint that already logs all the previous calltree information, so this additional log only makes our log files bigger.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] GSharayu commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
GSharayu commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r941931998
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "unknown";
+ // If reverse proxy is used X-Forwarded-For will be populated
+ // If X-Forwarded-For is not present, check if x-real-ip is present
+ // Since X-Forwarded-For can contain comma separated list of values, we convert it to ";" delimiter to avoid
+ // downstream parsing errors for other fields where "," is being used
+ if (enableClientIpLogging && requesterIdentity != null) {
+ for (Map.Entry<String, String> entry : ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().entries()) {
Review Comment:
I see we are assuming requesterIdentity of type HttpRequesterIdentity. We have GrpcRequesterIdentity as well and we are seeing query failure internally because of this change as we have our own custom requesterIdentity. @mcvsubbu @jtao15 @Jackie-Jiang .
@abhs50 We need to check if the requesterIdentity is instance of HttpRequesterIdentity?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r927944568
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
Review Comment:
How about "unknown" which is more clear?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8907:
URL: https://github.com/apache/pinot/pull/8907#issuecomment-1160847419
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8907?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8907](https://codecov.io/gh/apache/pinot/pull/8907?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92d29d6) into [master](https://codecov.io/gh/apache/pinot/commit/f3bde9ff674bc50ca296e22a0a8df2ded0168fa1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f3bde9f) will **decrease** coverage by `54.76%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #8907 +/- ##
=============================================
- Coverage 69.60% 14.84% -54.77%
+ Complexity 4997 170 -4827
=============================================
Files 1806 1766 -40
Lines 94202 92466 -1736
Branches 14050 13887 -163
=============================================
- Hits 65571 13725 -51846
- Misses 24072 77753 +53681
+ Partials 4559 988 -3571
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.84% <0.00%> (-0.59%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8907?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/8907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `32.76% <0.00%> (-40.50%)` | :arrow_down: |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/8907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/StringUtil.java](https://codecov.io/gh/apache/pinot/pull/8907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvU3RyaW5nVXRpbC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/8907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/8907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1395 more](https://codecov.io/gh/apache/pinot/pull/8907/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8907?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8907?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f3bde9f...92d29d6](https://codecov.io/gh/apache/pinot/pull/8907?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r899419574
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -682,7 +685,9 @@ 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(),
+ ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().toString(),
Review Comment:
I agree. Hence the comment here: https://github.com/apache/pinot/issues/8869#issuecomment-1157729246
Wanted to see what the community feels about this. If we have specific keys then it may only solve for certain set of use cases
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r899419574
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -682,7 +685,9 @@ 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(),
+ ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().toString(),
Review Comment:
I agree. Hence the comment here: https://github.com/apache/pinot/issues/8869#issuecomment-1157729246
Wanted to see what the community feels about this. If we have specific keys then it may only solve for a certain set of use cases. For example, if we add "X-Forwarded-For" for tracking source info, there might be use-cases where pinot users might be interested in custom headers specific to individual use case.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r906276792
##########
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:
Running it locally is an option. The broker log file should contain this string.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r918462453
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,32 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
+ // If reverse proxy is used X-Forwarded-For will be populated
+ // If X-Forwarded-For is not present, check if x-real-ip is present
+ // Since X-Forwarded-For can contain comma separated list of values, we convert it to ";" delimiter to avoid
+ // downstream parsing errors for other fields where "," is being used
+ if (enableClientIpLogging && requesterIdentity != null) {
+ for (Map.Entry<String, String> entries : ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().entries()) {
+ if (entries.getKey().equalsIgnoreCase("x-forwarded-for")) {
Review Comment:
Addressed here : https://github.com/apache/pinot/pull/8907/commits/3a68b33221a3079a54361ae32fb9db8eced9f468
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r933572789
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
Review Comment:
@abhs50 Let's do `unknown` for clarity since the line is already very long.
FYI #9122 is also touching this part, so if that one gets merged you might need to solve the conflict
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] GSharayu commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
GSharayu commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r941931998
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "unknown";
+ // If reverse proxy is used X-Forwarded-For will be populated
+ // If X-Forwarded-For is not present, check if x-real-ip is present
+ // Since X-Forwarded-For can contain comma separated list of values, we convert it to ";" delimiter to avoid
+ // downstream parsing errors for other fields where "," is being used
+ if (enableClientIpLogging && requesterIdentity != null) {
+ for (Map.Entry<String, String> entry : ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().entries()) {
Review Comment:
I see we are assuming requesterIdentity of type HttpRequesterIdentity. We have GrpcRequesterIdentity as well and we are seeing query failure internally because of this change as we have custom requesterIdentity. @mcvsubbu @jtao15 .
We need to check if the requesterIdentity is instance of HttpRequesterIdentity?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "unknown";
+ // If reverse proxy is used X-Forwarded-For will be populated
+ // If X-Forwarded-For is not present, check if x-real-ip is present
+ // Since X-Forwarded-For can contain comma separated list of values, we convert it to ";" delimiter to avoid
+ // downstream parsing errors for other fields where "," is being used
+ if (enableClientIpLogging && requesterIdentity != null) {
+ for (Map.Entry<String, String> entry : ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().entries()) {
Review Comment:
I see we are assuming requesterIdentity of type HttpRequesterIdentity. We have GrpcRequesterIdentity as well and we are seeing query failure internally because of this change as we have our own custom requesterIdentity. @mcvsubbu @jtao15 .
We need to check if the requesterIdentity is instance of HttpRequesterIdentity?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on PR #8907:
URL: https://github.com/apache/pinot/pull/8907#issuecomment-1181104613
LGTM. @siddharthteotia can you please review, kick-off tests and merge this PR?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mcvsubbu commented on pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on PR #8907:
URL: https://github.com/apache/pinot/pull/8907#issuecomment-1158140780
Please provide an example of what you see as the log entry, and then we can debate on the delimiter, format, etc.
Also, I want to make sure (like I indicated in the issue) that this is a MUST have feature before we start adding more stuff to the logs. I agree it is nice to have, but in large volume query systems, it can harm more than being useful
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r933641715
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
Review Comment:
Updated.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r903158186
##########
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:
Whats the easiest way to get this ? We are currently using helm chart and having this change propagated to docker seems like a not an easy path. Should I run this locally using docker ?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r906395996
##########
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:
I had this implementation originally, modified based on feedback : https://github.com/apache/pinot/pull/8907#discussion_r899427901
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] GSharayu commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
GSharayu commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r941931998
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "unknown";
+ // If reverse proxy is used X-Forwarded-For will be populated
+ // If X-Forwarded-For is not present, check if x-real-ip is present
+ // Since X-Forwarded-For can contain comma separated list of values, we convert it to ";" delimiter to avoid
+ // downstream parsing errors for other fields where "," is being used
+ if (enableClientIpLogging && requesterIdentity != null) {
+ for (Map.Entry<String, String> entry : ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().entries()) {
Review Comment:
I see we are assuming requesterIdentity of type HttpRequesterIdentity. We are seeing query failure internally because of this change as we have our own custom requesterIdentity. @mcvsubbu @jtao15 @Jackie-Jiang .
@abhs50 We need to check if the requesterIdentity is instance of HttpRequesterIdentity?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r899442872
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -682,7 +685,9 @@ 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(),
+ ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().toString(),
Review Comment:
Makes sense. I will change it to have only "X-Forwarded-For" as the key.
Any recommendations on which delimiter to you use to extract values part of the header. Example : ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().get("X-Forwarded-For") returns Collection<String> and adding the list to the log might conflict with the `,` separated format
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on PR #8907:
URL: https://github.com/apache/pinot/pull/8907#issuecomment-1190549618
@vvivekiyer : Any updates for next steps here ? Thanks
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on PR #8907:
URL: https://github.com/apache/pinot/pull/8907#issuecomment-1192734557
@abhs50 No action needed from you. Siddharth with review when he finds time and we can take it from there.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on PR #8907:
URL: https://github.com/apache/pinot/pull/8907#issuecomment-1170526857
Multiple IP Request :
`curl --location --request POST 'http://localhost:8000/query/sql' \
--header 'X-Forwarded-For: 103.0.113.165,60.91.3.17,120.192.338.678' \
--header 'Content-Type: text/plain' \
--data-raw '{"sql":"select yearID, count(*) from baseballStats group by yearID limit 20"}'`
Response :
<img width="1732" alt="pinot-log" src="https://user-images.githubusercontent.com/518685/176551180-c302b4fc-b761-4ad7-916e-d9a1d37f6836.png">
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on PR #8907:
URL: https://github.com/apache/pinot/pull/8907#issuecomment-1170528563
Single IP Request :
`curl --location --request POST 'http://localhost:8000/query/sql' \
--header 'X-Forwarded-For: 103.0.113.165' \
--header 'Content-Type: text/plain' \
--data-raw '{"sql":"select yearID, count(*) from baseballStats group by yearID limit 20"}'`
Response :
<img width="1749" alt="pinot-2-log" src="https://user-images.githubusercontent.com/518685/176551647-6a985f2d-c358-4e42-ae94-485f36434da2.png">
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r918430094
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,32 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
+ // If reverse proxy is used X-Forwarded-For will be populated
+ // If X-Forwarded-For is not present, check if x-real-ip is present
+ // Since X-Forwarded-For can contain comma separated list of values, we convert it to ";" delimiter to avoid
+ // downstream parsing errors for other fields where "," is being used
+ if (enableClientIpLogging && requesterIdentity != null) {
+ for (Map.Entry<String, String> entries : ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().entries()) {
Review Comment:
nit: entry
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #8907:
URL: https://github.com/apache/pinot/pull/8907
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] GSharayu commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
GSharayu commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r941931998
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "unknown";
+ // If reverse proxy is used X-Forwarded-For will be populated
+ // If X-Forwarded-For is not present, check if x-real-ip is present
+ // Since X-Forwarded-For can contain comma separated list of values, we convert it to ";" delimiter to avoid
+ // downstream parsing errors for other fields where "," is being used
+ if (enableClientIpLogging && requesterIdentity != null) {
+ for (Map.Entry<String, String> entry : ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().entries()) {
Review Comment:
I see we are assuming requesterIdentity of type HttpRequesterIdentity. We are seeing query failure internally because of this change as we have custom requesterIdentity. @mcvsubbu @jtao15 .
We need to check if the requesterIdentity is instance of HttpRequesterIdentity?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r903158186
##########
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:
Whats the easiest way to get this ? We are currently using helm chart and having this change propagated to docker repo seems like time consuming path. Should I run this locally using docker ? If not can you share steps on how to test this change.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on PR #8907:
URL: https://github.com/apache/pinot/pull/8907#issuecomment-1170573727
@vvivekiyer : Please review and provide comments. thanks !
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r903158186
##########
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:
Whats the easiest way to get this ? We are currently using helm chart and having this change propagated to docker repo seems like time consuming path. Should I run this locally using docker ?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r906279935
##########
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:
My suggestion is to pass requestIdentity to logBrokerResponse() instead of passing clientIps.
`logBrokerResponse(long requestId, ..., @Nullable RequesterIdentity requesterIdentity) `
In logBrokerResponse, we can extract clientIps only if the property is set and rateLimiter decides to log the query.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r918430353
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,32 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
+ // If reverse proxy is used X-Forwarded-For will be populated
+ // If X-Forwarded-For is not present, check if x-real-ip is present
+ // Since X-Forwarded-For can contain comma separated list of values, we convert it to ";" delimiter to avoid
+ // downstream parsing errors for other fields where "," is being used
+ if (enableClientIpLogging && requesterIdentity != null) {
+ for (Map.Entry<String, String> entries : ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().entries()) {
+ if (entries.getKey().equalsIgnoreCase("x-forwarded-for")) {
Review Comment:
nit: maybe store getKey() and getValue() to use them later. They are being used multiple times.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r931329999
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
Review Comment:
Is the community decided on which one to pick : 0, unknown, XXX, X ? I can make the change
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] GSharayu commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
GSharayu commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r941931998
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "unknown";
+ // If reverse proxy is used X-Forwarded-For will be populated
+ // If X-Forwarded-For is not present, check if x-real-ip is present
+ // Since X-Forwarded-For can contain comma separated list of values, we convert it to ";" delimiter to avoid
+ // downstream parsing errors for other fields where "," is being used
+ if (enableClientIpLogging && requesterIdentity != null) {
+ for (Map.Entry<String, String> entry : ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().entries()) {
Review Comment:
I see we are assuming requesterIdentity of type HttpRequesterIdentity. We have GrpcRequesterIdentity as well and we are seeing query failure internally because of this change as we have our own custom requesterIdentity. @mcvsubbu @jtao15 .
@Jackie-Jiang We need to check if the requesterIdentity is instance of HttpRequesterIdentity?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r928171375
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
Review Comment:
Understood. In regular case the ip should be longer than 7 characters, so I feel it is worth spending 6 extra characters and make the log more clear. User might be confused when seeing an ip as `0`
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r940544395
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
Review Comment:
@Jackie-Jiang : Are we good to go on this one ? thanks
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mcvsubbu commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r928277796
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
Review Comment:
Sure, I can settle for UNKNOWN
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r933641715
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
Review Comment:
@Jackie-Jiang : Updated.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mcvsubbu commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r927948441
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
Review Comment:
I was thinking to minimize the line length
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mcvsubbu commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r928277903
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,9 +661,34 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ @Nullable RequesterIdentity requesterIdentity) {
LOGGER.debug("Broker Response: {}", brokerResponse);
+ boolean enableClientIpLogging = _config.getProperty(Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING);
+ String clientIp = "";
Review Comment:
Or, "XXX" or just "X"?
I don't know, pick one. I think the line is getting a bit too long... but sure.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mcvsubbu commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r899427901
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -682,7 +685,9 @@ 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(),
+ ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().toString(),
Review Comment:
The idea of keeping the format as "key=value" and then a space and then the query is so that we can write scripts for post-processing, while looking at logs in production easily. If the value has spaces in it, that becomes a problem. It will certainly break some of the scripts we use for debugging.
If we are adding anything like a generic string that may have spaces, I suggest putting a marker AFTER the query, and adding the information there.
Also, we don't want to make the line too long.
Suggestions to add IP addresses (known not to have spaces in them) is good, and if that suffices then we can always add it in the "key=value" section. But please do name it correctly, instead of calling it "RequesterIdentity". This class can mean different in different environments, so please don't add this in the log message
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhs50 commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
abhs50 commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r903157376
##########
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:
requesterIdentity has a lot of params can mean different things based on which class is being used. Actually does it make sense to skip logging in this particular case ?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8907: Log Request header for Broker
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8907:
URL: https://github.com/apache/pinot/pull/8907#discussion_r899399245
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -682,7 +685,9 @@ 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(),
+ ((HttpRequesterIdentity) requesterIdentity).getHttpHeaders().toString(),
Review Comment:
Should we just use the host (key,value) from the headers instead of dumping all the headers here? I'm asking because certain header fields (like "accept-language") might not be really useful (at least right now).
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -658,7 +659,8 @@ private boolean isFilterAlwaysTrue(PinotQuery pinotQuery) {
}
private void logBrokerResponse(long requestId, String query, RequestContext requestContext, String tableName,
- int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs) {
+ int numUnavailableSegments, ServerStats serverStats, BrokerResponseNative brokerResponse, long totalTimeMs,
+ RequesterIdentity requesterIdentity) {
Review Comment:
requesterIdentity is a Nullable parameter. Can we add the annotation? It should probably be handled below to avoid nullPtrException?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org