Add some multi-stage metrics #12982
Add some multi-stage metrics #12982
Conversation
Including: - Report BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED - Report QUERY_TOTAL_TIME_MS - Report QUERY_QUOTA_EXCEEDED - Report REQUEST_DROPPED_DUE_TO_ACCESS_ERROR - Report QUERIES - Report REQUEST_SIZE - Add and report new MULTI_STAGE_QUERIES_BY_TABLE Some javadocs have been added to notice that some of these metrics can now be counted now more than once (ie BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED is added for each table touched in the query in case of joins)
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
Outdated
Show resolved
Hide resolved
| for (String tableName : tableNames) { | ||
| _brokerMetrics.addMeteredTableValue(tableName, BrokerMeter.BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED, 1); | ||
| } |
There was a problem hiding this comment.
We were planning to do something similar before and had some reservations around this. More info on this thread -- #11023 (comment)
cc @ankitsultana
There was a problem hiding this comment.
I think it is better to have false positives (aka report the problem at least once) than false negatives (aka report it at most once), but given there is no consensus, I'm going to remove it.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12982 +/- ##
============================================
+ Coverage 61.75% 62.16% +0.41%
+ Complexity 207 198 -9
============================================
Files 2436 2502 +66
Lines 133233 136592 +3359
Branches 20636 21146 +510
============================================
+ Hits 82274 84911 +2637
- Misses 44911 45409 +498
- Partials 6048 6272 +224
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
I've simplified the PR and modified the title and description of the PR. |
Jackie-Jiang
added
metrics
observability
multi-stage
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java
Outdated
Show resolved
Hide resolved
| * Unlike {@link #MULTI_STAGE_QUERIES_BY_TABLE}, this metric is global and not attached to a particular table. | ||
| * That means it can be used to know how many multi-stage queries have been started in total. | ||
| */ | ||
| MULTI_STAGE_QUERIES_EXECUTED("queries", true), |
There was a problem hiding this comment.
Suggest renaming them to MULTI_STAGE_QUERIES_GLOBAL and MULTI_STAGE_QUERIES
There was a problem hiding this comment.
Also way may put them next to QUERIES
There was a problem hiding this comment.
Also way may put them next to QUERIES
In general I try to avoid to change the order of literals to make sure backward compatibility is not broken, but I think in this case it is fine to do it.
Changing it...
This PR adds two new metrics:
Given a single multi-stage query may touch more than one table,
sum(MULTI_STAGE_QUERIES_BY_TABLE) >= MULTI_STAGE_QUERIES_EXECUTED.Originally this commit included the following changes, but I decided to rollback them to reduce the discussion and merge it sooner.