You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "rhodo (via GitHub)" <gi...@apache.org> on 2026/01/05 23:19:49 UTC
[PR] Broker: add SLA-style per-query error metrics [pinot]
rhodo opened a new pull request, #17457:
URL: https://github.com/apache/pinot/pull/17457
## Summary
Introduce two global broker meters to provide a 1:1, SLA-friendly view of query failures:
- querySystemError
- queryUserError
These are emitted once per query response that contains any exception, alongside the existing per-error-code meters.
## Motivation
Per-error-code meters can double-count when a single query surfaces multiple error codes. For SLA/reporting, a single per-query failure signal is desirable. This change adds a clean per-query classification while preserving existing detailed error metrics.
## Changes
- Add QUERY_SYSTEM_ERROR and QUERY_USER_ERROR in `BrokerMeter`
- Update `BrokerResponse.emitBrokerResponseMetrics` to:
- Continue emitting existing per-error-code meters for each exception.
- Emit per query querySystemError or queryUserError
- Centralize system error classification as a static `SYSTEM_ERROR_CODES` set on `BrokerResponse`.
## System error classification
A query is counted as system error if any exception has one of:
- SQL_RUNTIME
- INTERNAL
- QUERY_SCHEDULING_TIMEOUT
- EXECUTION_TIMEOUT
- BROKER_TIMEOUT
- SERVER_SEGMENT_MISSING
- BROKER_SEGMENT_UNAVAILABLE
- SERVER_NOT_RESPONDING
- BROKER_REQUEST_SEND
- MERGE_RESPONSE
- QUERY_CANCELLATION
- SERVER_SHUTTING_DOWN
- QUERY_PLANNING
All other error codes are counted as user errors.
--
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] Broker: add SLA-style per-query error metrics [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #17457:
URL: https://github.com/apache/pinot/pull/17457
--
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] Broker: add SLA-style per-query error metrics [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17457:
URL: https://github.com/apache/pinot/pull/17457#discussion_r2663668164
##########
pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java:
##########
@@ -41,6 +42,21 @@
*/
public interface BrokerResponse {
static final Logger LOGGER = LoggerFactory.getLogger(BrokerResponse.class);
+ static final EnumSet<QueryErrorCode> SYSTEM_ERROR_CODES = EnumSet.of(
Review Comment:
QueryErrorCode has a field of `_httpResponseStatus`
Can we use this code to distinguish the user errors(4XX) and internal errors(5XX)?
Introducing a new `SYSTEM_ERROR_CODES` will make it harder to maintain.
--
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] Broker: add SLA-style per-query error metrics [pinot]
Posted by "rhodo (via GitHub)" <gi...@apache.org>.
rhodo commented on code in PR #17457:
URL: https://github.com/apache/pinot/pull/17457#discussion_r2666795801
##########
pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java:
##########
@@ -76,6 +92,8 @@ default String toMetadataJsonString()
* This method ensures we emit metrics for all queries that have exceptions with a one-to-one mapping.
*/
default void emitBrokerResponseMetrics(BrokerMetrics brokerMetrics) {
+ boolean isSystemError = false;
+ boolean isUserError = false;
Review Comment:
each metrics is 1:1 mapping, but system/user (critical/noncritical) is not mutual exclusive as a query can contains multiple errors.
--
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] Broker: add SLA-style per-query error metrics [pinot]
Posted by "rhodo (via GitHub)" <gi...@apache.org>.
rhodo commented on PR #17457:
URL: https://github.com/apache/pinot/pull/17457#issuecomment-3716999118
renaming the metrics to critical/non-critical to better reflect the intention. Move the definition into QueryErrorCode.java
--
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] Broker: add SLA-style per-query error metrics [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17457:
URL: https://github.com/apache/pinot/pull/17457#issuecomment-3712537399
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17457?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `23.07692%` with `10 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.57%. Comparing base ([`042549d`](https://app.codecov.io/gh/apache/pinot/commit/042549dd64508852c3e4edb24942c08b3499f720?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f4bb94c`](https://app.codecov.io/gh/apache/pinot/commit/f4bb94c3492344fd325e029caabffa53034ea19b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17457?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...g/apache/pinot/common/response/BrokerResponse.java](https://app.codecov.io/gh/apache/pinot/pull/17457?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fresponse%2FBrokerResponse.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzcG9uc2UvQnJva2VyUmVzcG9uc2UuamF2YQ==) | 9.09% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17457?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 (042549d) and HEAD (f4bb94c). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (042549d) | HEAD (f4bb94c) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17457 +/- ##
============================================
- Coverage 63.34% 55.57% -7.77%
+ Complexity 1474 702 -772
============================================
Files 3162 2470 -692
Lines 188591 139478 -49113
Branches 28857 22237 -6620
============================================
- Hits 119454 77512 -41942
+ Misses 59891 55436 -4455
+ Partials 9246 6530 -2716
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17457/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/17457/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/17457/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/17457/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/17457/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/17457/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/17457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.57% <23.07%> (-7.70%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.57% <23.07%> (-7.77%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.57% <23.07%> (-7.77%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.57% <23.07%> (-0.05%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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/17457?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>
- :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] Broker: add SLA-style per-query error metrics [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17457:
URL: https://github.com/apache/pinot/pull/17457#discussion_r2663668164
##########
pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java:
##########
@@ -41,6 +42,21 @@
*/
public interface BrokerResponse {
static final Logger LOGGER = LoggerFactory.getLogger(BrokerResponse.class);
+ static final EnumSet<QueryErrorCode> SYSTEM_ERROR_CODES = EnumSet.of(
Review Comment:
QueryErrorCode has a field of `_httpResponseStatus`
Can we use this code to distinguish the user errors(4XX) and internal errors(5XX)?
--
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] Broker: add SLA-style per-query error metrics [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17457:
URL: https://github.com/apache/pinot/pull/17457#discussion_r2663658987
##########
pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java:
##########
@@ -41,6 +42,21 @@
*/
public interface BrokerResponse {
static final Logger LOGGER = LoggerFactory.getLogger(BrokerResponse.class);
+ static final EnumSet<QueryErrorCode> SYSTEM_ERROR_CODES = EnumSet.of(
Review Comment:
The `SYSTEM_ERROR_CODES` field should have an explicit visibility modifier. According to Java best practices, interface fields are public, static, and final by default, but explicitly declaring `public` improves code clarity and maintainability.
```suggestion
public static final EnumSet<QueryErrorCode> SYSTEM_ERROR_CODES = EnumSet.of(
```
##########
pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java:
##########
@@ -76,6 +92,8 @@ default String toMetadataJsonString()
* This method ensures we emit metrics for all queries that have exceptions with a one-to-one mapping.
*/
default void emitBrokerResponseMetrics(BrokerMetrics brokerMetrics) {
+ boolean isSystemError = false;
+ boolean isUserError = false;
Review Comment:
If a query has both system and user errors, both `isSystemError` and `isUserError` will be true, resulting in both metrics being incremented for the same query. This contradicts the PR's goal of providing a single per-query classification. Consider using mutually exclusive classification logic, such as prioritizing system errors over user errors, or emitting only one metric based on the most severe error type.
##########
pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java:
##########
@@ -85,6 +103,18 @@ default void emitBrokerResponseMetrics(BrokerMetrics brokerMetrics) {
queryErrorCode = QueryErrorCode.UNKNOWN;
}
brokerMetrics.addMeteredGlobalValue(BrokerMeter.getQueryErrorMeter(queryErrorCode), 1);
+ if (SYSTEM_ERROR_CODES.contains(queryErrorCode)) {
+ isSystemError = true;
+ } else {
+ isUserError = true;
+ }
Review Comment:
The `UNKNOWN` error code (set at line 103) will always be classified as a user error since it's not in `SYSTEM_ERROR_CODES`. This may not be the intended behavior for truly unknown errors. Consider explicitly handling `QueryErrorCode.UNKNOWN` or documenting this classification decision.
--
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