-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multi stage metrics #13035
Multi stage metrics #13035
Conversation
44017a0
to
14d0b58
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13035 +/- ##
============================================
+ Coverage 61.75% 62.05% +0.30%
+ Complexity 207 198 -9
============================================
Files 2436 2544 +108
Lines 133233 139740 +6507
Branches 20636 21611 +975
============================================
+ Hits 82274 86720 +4446
- Misses 44911 46532 +1621
- Partials 6048 6488 +440
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
14d0b58
to
8637119
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerMeter.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java
Outdated
Show resolved
Hide resolved
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/MultiStageQueryStats.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
} | ||
} | ||
|
||
public List<StageStats.Closed> getClosedStats() { | ||
return Collections.unmodifiableList(_closedStats); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is internal only code, we can skip wrapping into unmodifiable to reduce overhead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A unmodifiable list is very light (just an indirection) and TBH I prefer to be a bit paranoic here so we don't end up modifying it in the future, which would be difficult to catch.
This method is only called once per query on the broker. I don't think creating a single small object (that can be probably scalar replaced) would be problematic.
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR adds some multi-stage metrics. The whole PR is built on top of #12704 so it should only be merged once (and if) that other PR is merged.
All new metrics are server based. Specifically, they are:
Meters:
Timers