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/21 01:03:24 UTC
[GitHub] [pinot] navina opened a new pull request, #8941: Defined a new broker metric for total query processing time
navina opened a new pull request, #8941:
URL: https://github.com/apache/pinot/pull/8941
### What does this PR do?
Defined a new broker metric for total query processing time and added metric to broker.yaml
Tags to include: `observability` , `release notes`
### Release Notes:
Includes a new broker gauge type metric - `queryTotalTimeMs`
--
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] navina commented on a diff in pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #8941:
URL: https://github.com/apache/pinot/pull/8941#discussion_r902217883
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -585,6 +586,7 @@ private BrokerResponseNative handleRequest(long requestId, String query, JsonNod
brokerResponse.setTimeUsedMs(totalTimeMs);
requestContext.setQueryProcessingTime(totalTimeMs);
augmentStatistics(requestContext, brokerResponse);
+ _brokerMetrics.setValueOfTableGauge(rawTableName, BrokerGauge.QUERY_TOTAL_TIME_MS, totalTimeMs);
Review Comment:
yeah. I wasn't sure if this falls under the "query phase" metrics as it seemed to be applicable for each stage. I don't think percentiles/average etc are related to `BrokerQueryPhase`, but rather to the metric type itself ?
Please correct me if I have misunderstood. :)
--
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] navina commented on a diff in pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #8941:
URL: https://github.com/apache/pinot/pull/8941#discussion_r904289245
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -585,6 +586,7 @@ private BrokerResponseNative handleRequest(long requestId, String query, JsonNod
brokerResponse.setTimeUsedMs(totalTimeMs);
requestContext.setQueryProcessingTime(totalTimeMs);
augmentStatistics(requestContext, brokerResponse);
+ _brokerMetrics.setValueOfTableGauge(rawTableName, BrokerGauge.QUERY_TOTAL_TIME_MS, totalTimeMs);
Review Comment:
`Timer` has rates and distribution stats. `Gauges` don't. (https://metrics.dropwizard.io/4.2.0/getting-started.html#timers)[https://metrics.dropwizard.io/4.2.0/getting-started.html#timers]
I also verified the metrics locally - it is seen in mbeans and getting published to prometheus.
--
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] npawar commented on a diff in pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
npawar commented on code in PR #8941:
URL: https://github.com/apache/pinot/pull/8941#discussion_r904210332
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -585,6 +586,7 @@ private BrokerResponseNative handleRequest(long requestId, String query, JsonNod
brokerResponse.setTimeUsedMs(totalTimeMs);
requestContext.setQueryProcessingTime(totalTimeMs);
augmentStatistics(requestContext, brokerResponse);
+ _brokerMetrics.setValueOfTableGauge(rawTableName, BrokerGauge.QUERY_TOTAL_TIME_MS, totalTimeMs);
Review Comment:
just thought it makes more sense here, as all the phases are here, and some of them are already supersets of the others. And now this one is the ultimate superset:
```
REQUEST_COMPILATION,
QUERY_EXECUTION,
QUERY_ROUTING,
SCATTER_GATHER,
DESERIALIZATION,
```
We do see the aggregations applied to these afaik..
--
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] kishoreg commented on pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
kishoreg commented on PR #8941:
URL: https://github.com/apache/pinot/pull/8941#issuecomment-1161287043
How is this different from the totaltimems that we have today?
--
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] navina commented on pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
navina commented on PR #8941:
URL: https://github.com/apache/pinot/pull/8941#issuecomment-1161329788
> How is this different from the totaltimems that we have today?
Are you referring to `BrokerQueryPhase.QUERY_EXECUTION` metric? It is currently defined as:
```
_brokerMetrics.addPhaseTiming(rawTableName, BrokerQueryPhase.QUERY_EXECUTION,
executionEndTimeNs - routingEndTimeNs);
```
So, it is not the total query processing time. Total query time should start from the request compilation phase and all the way to the end.
--
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] navina commented on a diff in pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #8941:
URL: https://github.com/apache/pinot/pull/8941#discussion_r902218078
##########
pinot-controller/src/main/resources/package-lock.json:
##########
@@ -12,14 +12,6 @@
"@babel/highlight": "^7.16.7"
}
},
- "@babel/helper-module-imports": {
Review Comment:
urgh.. i thought I removed this from the commit. will fix it. tks!
--
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] siddharthteotia commented on a diff in pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8941:
URL: https://github.com/apache/pinot/pull/8941#discussion_r903134331
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -585,6 +586,7 @@ private BrokerResponseNative handleRequest(long requestId, String query, JsonNod
brokerResponse.setTimeUsedMs(totalTimeMs);
requestContext.setQueryProcessingTime(totalTimeMs);
augmentStatistics(requestContext, brokerResponse);
+ _brokerMetrics.setValueOfTableGauge(rawTableName, BrokerGauge.QUERY_TOTAL_TIME_MS, totalTimeMs);
Review Comment:
To confirm - the new metric will include the total latency observed at broker right from the time of arrival and all the other phases ?
--
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] navina commented on pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
navina commented on PR #8941:
URL: https://github.com/apache/pinot/pull/8941#issuecomment-1165823464
@npawar can you 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] npawar merged pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
npawar merged PR #8941:
URL: https://github.com/apache/pinot/pull/8941
--
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] npawar commented on a diff in pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
npawar commented on code in PR #8941:
URL: https://github.com/apache/pinot/pull/8941#discussion_r902801326
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -585,6 +586,7 @@ private BrokerResponseNative handleRequest(long requestId, String query, JsonNod
brokerResponse.setTimeUsedMs(totalTimeMs);
requestContext.setQueryProcessingTime(totalTimeMs);
augmentStatistics(requestContext, brokerResponse);
+ _brokerMetrics.setValueOfTableGauge(rawTableName, BrokerGauge.QUERY_TOTAL_TIME_MS, totalTimeMs);
Review Comment:
i think the aggregations dont apply to Gauges. You might want to confirm that. If they do, then looks fine
--
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] navina commented on a diff in pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #8941:
URL: https://github.com/apache/pinot/pull/8941#discussion_r904100656
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -585,6 +586,7 @@ private BrokerResponseNative handleRequest(long requestId, String query, JsonNod
brokerResponse.setTimeUsedMs(totalTimeMs);
requestContext.setQueryProcessingTime(totalTimeMs);
augmentStatistics(requestContext, brokerResponse);
+ _brokerMetrics.setValueOfTableGauge(rawTableName, BrokerGauge.QUERY_TOTAL_TIME_MS, totalTimeMs);
Review Comment:
@siddharthteotia yes.. that's right.
@npawar I think we need to use a `Timer` metric type if you need percentiles and average. I can change to use that. `BrokerQueryPhase` just labels the phase name to the metric name. Under the hood, it is a timer metric.
--
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] navina commented on pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
navina commented on PR #8941:
URL: https://github.com/apache/pinot/pull/8941#issuecomment-1161017640
Review pls : @npawar
--
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] npawar commented on a diff in pull request #8941: Defined a new broker metric for total query processing time
Posted by GitBox <gi...@apache.org>.
npawar commented on code in PR #8941:
URL: https://github.com/apache/pinot/pull/8941#discussion_r902170670
##########
pinot-controller/src/main/resources/package-lock.json:
##########
@@ -12,14 +12,6 @@
"@babel/highlight": "^7.16.7"
}
},
- "@babel/helper-module-imports": {
Review Comment:
revert this file?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -585,6 +586,7 @@ private BrokerResponseNative handleRequest(long requestId, String query, JsonNod
brokerResponse.setTimeUsedMs(totalTimeMs);
requestContext.setQueryProcessingTime(totalTimeMs);
augmentStatistics(requestContext, brokerResponse);
+ _brokerMetrics.setValueOfTableGauge(rawTableName, BrokerGauge.QUERY_TOTAL_TIME_MS, totalTimeMs);
Review Comment:
should this be a BrokerQueryPhase (in addition to Gauge) so we can get the percentiles, average etc?
--
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