You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "krishan1390 (via GitHub)" <gi...@apache.org> on 2025/03/27 11:27:18 UTC
[PR] Emit metrics for each query error code returned by the broker [pinot]
krishan1390 opened a new pull request, #15387:
URL: https://github.com/apache/pinot/pull/15387
Adding metrics to all error codes returned by the broker.
Currently, the broker emits certain metrics for certain error codes but its not comprehensive nor accurate (there is a many to many relationship between error codes and metrics)
Rather than refactor the code, in this PR, we're adding new standardized metrics for query error codes from broker.
Will try to add similar metrics from server (which will help identify which server has an issue)
Note to reviewers:
1. PR tags to be added - metrics, observability
2. Let me know if anything else needs to be specifically tested
Testing Done:
1. Verified new metrics are present through jmxterm
```
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorAccessDenied",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorBrokerInstanceMissing",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorBrokerRequestSend",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorBrokerResourceMissing",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorBrokerSegmentUnavailable",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorBrokerTimeout",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorExecutionTimeout",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorInternal",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorJsonParsing",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorMergeResponse",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorQueryCancellation",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorQueryExecution",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorQueryPlanning",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorQuerySchedulingTimeout",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorQueryValidation",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorServerNotResponding",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorServerOutOfCapacity",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorServerResourceLimitExceeded",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorServerSegmentMissing",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorServerShuttingDown",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorServerTableMissing",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorSqlParsing",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorSqlRuntime",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorTableDoesNotExist",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorTableIsDisabled",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorTooManyRequests",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorUnknown",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorUnknownColumn",type="BrokerMetrics"
```
3. Verified count is increasing when issuing a wrong query
```
$>bean "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorUnknownColumn",type="BrokerMetrics"
#bean is set to "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorUnknownColumn",type="BrokerMetrics"
$>get Count
#mbean = "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorUnknownColumn",type="BrokerMetrics":
Count = 1;
$>get Count
#mbean = "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorUnknownColumn",type="BrokerMetrics":
Count = 2;
$>bean "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorTableDoesNotExist",type="BrokerMetrics"
#bean is set to "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorTableDoesNotExist",type="BrokerMetrics"
$>get Count
#mbean = "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorTableDoesNotExist",type="BrokerMetrics":
Count = 1;
$>get Count
#mbean = "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorTableDoesNotExist",type="BrokerMetrics":
Count = 2;
```
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2017899865
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -18,195 +18,134 @@
*/
package org.apache.pinot.common.metrics;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import org.apache.pinot.common.Utils;
-
+import org.apache.pinot.spi.exception.QueryErrorCode;
/**
- * Enumeration containing all the metrics exposed by the Pinot broker.
+ * Class containing all the metrics exposed by the Pinot broker.
Review Comment:
I couldn't figure out another way apart from above mentioned. tried creating a seperate class but AbstractMetrics restricts that. and didn't want to add that option of manual mistake if someone adds an error code but misses adding a metric.
Yea the class structure changes, but the usage of the class remains same. Clients still get to use same variables as earlier without any changes. and adding new metrics is also same effort as before by just creaing a new variable.
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2016398193
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -18,195 +18,134 @@
*/
package org.apache.pinot.common.metrics;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import org.apache.pinot.common.Utils;
-
+import org.apache.pinot.spi.exception.QueryErrorCode;
/**
- * Enumeration containing all the metrics exposed by the Pinot broker.
+ * Class containing all the metrics exposed by the Pinot broker.
Review Comment:
Why is the change from enum to class required ?
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2016436112
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -560,6 +568,24 @@ public static Response getPinotQueryResponse(BrokerResponse brokerResponse)
.build();
}
+ // If a broker response has multiple exceptions, we will emit metrics for all of them.
+ // Thus, the sum total of all exceptions is >= total number of queries impacted.
+ // Additionally, some parts of code might already be emitting metrics for individual error codes.
+ // But that list isn't accurate with a many-to-many relationship (or no metrics) between error codes and metrics.
+ // This method ensures we emit metrics for all queries that have exceptions with a one-to-one mapping.
+ private void emitBrokerResponseMetrics(BrokerResponse brokerResponse) {
Review Comment:
Is metrics from servers useful ? The broker has the inteliigence to get all the exceptions. If a query is cancelled, then every server will report a cancellation.
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #15387:
URL: https://github.com/apache/pinot/pull/15387#issuecomment-2757862603
some of the errors already have meters. e.g.
```
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorBrokerResourceMissing",type="BrokerMetrics"
"org.apache.pinot.common.metrics":name="pinot.broker.queryErrorBrokerSegmentUnavailable",type="BrokerMetrics"
```
Maybe you need a list of unreported errors and only report those.
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on PR #15387:
URL: https://github.com/apache/pinot/pull/15387#issuecomment-2757934668
> `BrokerRequestHandlers` is a better place to capture these metrics. Many other metrics are reported there as well.
>
> https://github.com/apache/pinot/blob/master/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java#L783
>
> https://github.com/apache/pinot/blob/master/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java#L167
The problem with BrokerRequestHandlers is many SQL_PARSING errors are thrown before it reaches here. And few client side error codes too around authorization / quota limits etc. While some cases can be considered optional and OK to not track, its a choice. I took the option to track all error codes.
Let me know if you still think otherwise
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2017949534
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -18,195 +18,134 @@
*/
package org.apache.pinot.common.metrics;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import org.apache.pinot.common.Utils;
-
+import org.apache.pinot.spi.exception.QueryErrorCode;
/**
- * Enumeration containing all the metrics exposed by the Pinot broker.
+ * Class containing all the metrics exposed by the Pinot broker.
Review Comment:
tried creating a seperate class but AbstractMetrics restricts that.
thanks for the javadocs catch. I missed seeing it got removed
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2016319955
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -560,6 +568,24 @@ public static Response getPinotQueryResponse(BrokerResponse brokerResponse)
.build();
}
+ // If a broker response has multiple exceptions, we will emit metrics for all of them.
+ // Thus, the sum total of all exceptions is >= total number of queries impacted.
+ // Additionally, some parts of code might already be emitting metrics for individual error codes.
+ // But that list isn't accurate with a many-to-many relationship (or no metrics) between error codes and metrics.
+ // This method ensures we emit metrics for all queries that have exceptions with a one-to-one mapping.
+ private void emitBrokerResponseMetrics(BrokerResponse brokerResponse) {
Review Comment:
ideally would want to use the same code in server too so should I rather create a new class in org.apache.pinot.common.metrics for just this code ?
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15387:
URL: https://github.com/apache/pinot/pull/15387#issuecomment-2757816310
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15387?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `71.29630%` with `31 lines` in your changes missing coverage. Please review.
> Project coverage is 33.81%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`91b8236`)](https://app.codecov.io/gh/apache/pinot/commit/91b823693445920b94c769923ee727ed1e2e58b0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1928 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/15387?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pinot/broker/api/resources/PinotClientRequest.java](https://app.codecov.io/gh/apache/pinot/pull/15387?src=pr&el=tree&filepath=pinot-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fbroker%2Fapi%2Fresources%2FPinotClientRequest.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | 0.00% | [17 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15387?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/pinot/broker/grpc/BrokerGrpcServer.java](https://app.codecov.io/gh/apache/pinot/pull/15387?src=pr&el=tree&filepath=pinot-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fbroker%2Fgrpc%2FBrokerGrpcServer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvZ3JwYy9Ccm9rZXJHcnBjU2VydmVyLmphdmE=) | 0.00% | [13 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15387?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/org/apache/pinot/common/metrics/BrokerMeter.java](https://app.codecov.io/gh/apache/pinot/pull/15387?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fmetrics%2FBrokerMeter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJNZXRlci5qYXZh) | 98.71% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15387?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (91b8236). Click for more details.
>
> <details><summary>HEAD has 48 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (91b8236) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|2|
>|java-21|7|2|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|1|
>|unittests|5|2|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|2|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15387 +/- ##
=============================================
- Coverage 61.75% 33.81% -27.94%
- Complexity 207 684 +477
=============================================
Files 2436 2812 +376
Lines 133233 158916 +25683
Branches 20636 24329 +3693
=============================================
- Hits 82274 53738 -28536
- Misses 44911 100880 +55969
+ Partials 6048 4298 -1750
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.81% <71.29%> (-27.81%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.79% <71.29%> (-27.95%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.81% <71.29%> (+6.08%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.81% <71.29%> (-27.94%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.81% <71.29%> (-27.94%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.81% <71.29%> (+6.08%)` | :arrow_up: |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/15387?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2017024787
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -18,195 +18,134 @@
*/
package org.apache.pinot.common.metrics;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import org.apache.pinot.common.Utils;
-
+import org.apache.pinot.spi.exception.QueryErrorCode;
/**
- * Enumeration containing all the metrics exposed by the Pinot broker.
+ * Class containing all the metrics exposed by the Pinot broker.
Review Comment:
This is a big structural change in terms of dev u/x. Metrics for other components as well as in Startree follow the current pattern. So this will become an odd one out. Are there other ways to achieve what you need ?
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2017135992
##########
pinot-broker/src/main/java/org/apache/pinot/broker/grpc/BrokerGrpcServer.java:
##########
@@ -181,15 +182,17 @@ public void submit(Broker.BrokerRequest request,
try {
sqlNodeAndOptions = RequestUtils.parseQuery(query, requestJsonNode);
} catch (Exception e) {
- // Do not log or emit metric here because it is pure user error
Review Comment:
In this block, only `SQL_PARSING` error can be thrown. So a simpler change is to increment the specific meter ?
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #15387:
URL: https://github.com/apache/pinot/pull/15387
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on PR #15387:
URL: https://github.com/apache/pinot/pull/15387#issuecomment-2757926953
> some of the errors already have meters. e.g.
>
> ```
> "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorBrokerResourceMissing",type="BrokerMetrics"
> "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorBrokerSegmentUnavailable",type="BrokerMetrics"
> ```
>
> Maybe you need a list of unreported errors and only report those.
Yes some errors have appropriate meters like these. But a lot of errors don't and there is a many to many relationship between error codes and metric names. eg: Some SQL_PARSING and some QUERY_PLANNING error code emits a REQUEST_COMPILATION_EXCEPTIONS metric. Some REQUEST_COMPILATION_EXCEPTIONS are emitted for other use cases. There are many more such cases.
Some metrics might be important in their current form. So refactoring the metrics would be backward incompatible and take a lot of time.
I thought the simpler option is to create standard metrics in a standard path. This provides us clearer view on what error code is actually returned to the user. So, there will be some redundant metrics but I think thats ok given it moves towards standardization (metrics starting with pinot.broker.queryError)
Let me know if you still feel otherwise
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #15387:
URL: https://github.com/apache/pinot/pull/15387#issuecomment-2758614239
> > some of the errors already have meters. e.g.
> > ```
> > "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorBrokerResourceMissing",type="BrokerMetrics"
> > "org.apache.pinot.common.metrics":name="pinot.broker.queryErrorBrokerSegmentUnavailable",type="BrokerMetrics"
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Maybe you need a list of unreported errors and only report those.
>
> Yes some errors have appropriate meters like these. But a lot of errors don't and there is a many to many relationship between error codes and metric names. eg: Some SQL_PARSING and some QUERY_PLANNING error code emits a REQUEST_COMPILATION_EXCEPTIONS metric. Some REQUEST_COMPILATION_EXCEPTIONS are emitted for other use cases. There are many more such cases.
>
> Some metrics might be important in their current form. So refactoring the metrics would be backward incompatible and take a lot of time.
>
> I thought the simpler option is to create standard metrics in a standard path. This provides us clearer view on what error code is actually returned to the user. So, there will be some redundant metrics but I think thats ok given it moves towards standardization (metrics starting with pinot.broker.queryError)
>
> Let me know if you still feel otherwise
IIUC, there is no double counting ? There will be two metrics possibly that will track the same error type ? for e.g. `queryValidationError` and `queryErrorQueryValidation` ? That sounds reasonable.
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2017926330
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -18,195 +18,134 @@
*/
package org.apache.pinot.common.metrics;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import org.apache.pinot.common.Utils;
-
+import org.apache.pinot.spi.exception.QueryErrorCode;
/**
- * Enumeration containing all the metrics exposed by the Pinot broker.
+ * Class containing all the metrics exposed by the Pinot broker.
Review Comment:
I have validated the final metric names via comparing the bean names from this branch and master and doing a text diff
also compared unit and global flags are same for all variables manually.
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2018063221
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMetrics.java:
##########
@@ -84,4 +91,23 @@ protected BrokerMeter[] getMeters() {
protected BrokerGauge[] getGauges() {
return BrokerGauge.values();
}
+
+ // Emits metrics based on BrokerResponse. Currently only emits metrics for exceptions.
+ // If a broker response has multiple exceptions, we will emit metrics for all of them.
+ // Thus, the sum total of all exceptions is >= total number of queries impacted.
+ // Additionally, some parts of code might already be emitting metrics for individual error codes.
+ // But that list isn't accurate with a many-to-many relationship (or no metrics) between error codes and metrics.
+ // This method ensures we emit metrics for all queries that have exceptions with a one-to-one mapping.
+ public void emitBrokerResponseMetrics(BrokerResponse brokerResponse) {
Review Comment:
yea that was the other option. moved to BrokerResponse
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2018026339
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMetrics.java:
##########
@@ -84,4 +91,23 @@ protected BrokerMeter[] getMeters() {
protected BrokerGauge[] getGauges() {
return BrokerGauge.values();
}
+
+ // Emits metrics based on BrokerResponse. Currently only emits metrics for exceptions.
+ // If a broker response has multiple exceptions, we will emit metrics for all of them.
+ // Thus, the sum total of all exceptions is >= total number of queries impacted.
+ // Additionally, some parts of code might already be emitting metrics for individual error codes.
+ // But that list isn't accurate with a many-to-many relationship (or no metrics) between error codes and metrics.
+ // This method ensures we emit metrics for all queries that have exceptions with a one-to-one mapping.
+ public void emitBrokerResponseMetrics(BrokerResponse brokerResponse) {
Review Comment:
I think it is fine here, but please change the simple comment (`//`) to one of the javadoc formats (either the new `///` or the good old classic `/**/)
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2018025039
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -18,195 +18,134 @@
*/
package org.apache.pinot.common.metrics;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import org.apache.pinot.common.Utils;
-
+import org.apache.pinot.spi.exception.QueryErrorCode;
/**
- * Enumeration containing all the metrics exposed by the Pinot broker.
+ * Class containing all the metrics exposed by the Pinot broker.
Review Comment:
The current metric system is very restrictive. We need to be able to extend it in the future. I don't love this change, but I think it is necessary.
Anyway, we are not consuming this class as an enum (ie, applying switches on values of this type or using them as enums in binary representation) so it shouldn't affect us that much.
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2016312059
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -560,6 +568,24 @@ public static Response getPinotQueryResponse(BrokerResponse brokerResponse)
.build();
}
+ // If a broker response has multiple exceptions, we will emit metrics for all of them.
+ // Thus, the sum total of all exceptions is >= total number of queries impacted.
+ // Additionally, some parts of code might already be emitting metrics for individual error codes.
+ // But that list isn't accurate with a many-to-many relationship (or no metrics) between error codes and metrics.
+ // This method ensures we emit metrics for all queries that have exceptions with a one-to-one mapping.
+ private void emitBrokerResponseMetrics(BrokerResponse brokerResponse) {
Review Comment:
This method should be moved to a common place so that it can be reused in BrokerGrpcServer and other places.
what would be the right class for it ? BrokerResponse or BrokerMetrics or something else ?
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2016481851
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -560,6 +568,24 @@ public static Response getPinotQueryResponse(BrokerResponse brokerResponse)
.build();
}
+ // If a broker response has multiple exceptions, we will emit metrics for all of them.
+ // Thus, the sum total of all exceptions is >= total number of queries impacted.
+ // Additionally, some parts of code might already be emitting metrics for individual error codes.
+ // But that list isn't accurate with a many-to-many relationship (or no metrics) between error codes and metrics.
+ // This method ensures we emit metrics for all queries that have exceptions with a one-to-one mapping.
+ private void emitBrokerResponseMetrics(BrokerResponse brokerResponse) {
Review Comment:
Primary reason to get from server would be to identify which server is returning the error the most (if its an isolated issue). or to validate if the issue is across servers etc
eg: for errors like SERVER_TABLE_MISSING, SERVER_SEGMENT_MISSING, QUERY_SCHEDULING_TIMEOUT etc which could be possible on some servers
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2016483913
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -18,195 +18,134 @@
*/
package org.apache.pinot.common.metrics;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import org.apache.pinot.common.Utils;
-
+import org.apache.pinot.spi.exception.QueryErrorCode;
/**
- * Enumeration containing all the metrics exposed by the Pinot broker.
+ * Class containing all the metrics exposed by the Pinot broker.
Review Comment:
wanted to create metrics dynamically for all error codes to prevent issues where developers forget to add a metric when adding a new error code. we can't do that with enums.
and can't create a seperate class for that because AbstractMetrics can be created with just 1 instance of AbstractMetrics.Meter class
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2017897838
##########
pinot-broker/src/main/java/org/apache/pinot/broker/grpc/BrokerGrpcServer.java:
##########
@@ -181,15 +182,17 @@ public void submit(Broker.BrokerRequest request,
try {
sqlNodeAndOptions = RequestUtils.parseQuery(query, requestJsonNode);
} catch (Exception e) {
- // Do not log or emit metric here because it is pure user error
Review Comment:
yes in this block currently only SQL_PARSING can be thrown. made the change primarily to keep the flow consistent with all other changes where we send back a brokerResponse (sort of making it future proof if more error codes are added in this block)
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2017933728
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMetrics.java:
##########
@@ -84,4 +91,23 @@ protected BrokerMeter[] getMeters() {
protected BrokerGauge[] getGauges() {
return BrokerGauge.values();
}
+
+ // Emits metrics based on BrokerResponse. Currently only emits metrics for exceptions.
+ // If a broker response has multiple exceptions, we will emit metrics for all of them.
+ // Thus, the sum total of all exceptions is >= total number of queries impacted.
+ // Additionally, some parts of code might already be emitting metrics for individual error codes.
+ // But that list isn't accurate with a many-to-many relationship (or no metrics) between error codes and metrics.
+ // This method ensures we emit metrics for all queries that have exceptions with a one-to-one mapping.
+ public void emitBrokerResponseMetrics(BrokerResponse brokerResponse) {
Review Comment:
Let me know if this method should belong somewhere else
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2017945811
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -18,195 +18,134 @@
*/
package org.apache.pinot.common.metrics;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import org.apache.pinot.common.Utils;
-
+import org.apache.pinot.spi.exception.QueryErrorCode;
/**
- * Enumeration containing all the metrics exposed by the Pinot broker.
+ * Class containing all the metrics exposed by the Pinot broker.
Review Comment:
Is another option not to use this class ? Since you are generating metrics dynamically, create a separate class for that ? That seems like a clean separation.
wrt to dev u/x adding a metric is different.
One more thing - the javadocs have been removed.
--
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
Re: [PR] Emit metrics for each query error code returned by the broker [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15387:
URL: https://github.com/apache/pinot/pull/15387#discussion_r2018026339
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMetrics.java:
##########
@@ -84,4 +91,23 @@ protected BrokerMeter[] getMeters() {
protected BrokerGauge[] getGauges() {
return BrokerGauge.values();
}
+
+ // Emits metrics based on BrokerResponse. Currently only emits metrics for exceptions.
+ // If a broker response has multiple exceptions, we will emit metrics for all of them.
+ // Thus, the sum total of all exceptions is >= total number of queries impacted.
+ // Additionally, some parts of code might already be emitting metrics for individual error codes.
+ // But that list isn't accurate with a many-to-many relationship (or no metrics) between error codes and metrics.
+ // This method ensures we emit metrics for all queries that have exceptions with a one-to-one mapping.
+ public void emitBrokerResponseMetrics(BrokerResponse brokerResponse) {
Review Comment:
Please change the simple comment (`//`) to one of the javadoc formats (either the new `///` or the good old classic `/**/)
Thinking about the place... maybe it could be done in the opposite way. We can add a emitMetrics(BrokerMetrics) in BrokerResponse. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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