Skip to content
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

Add percentiles report in QuerySummary #11299

Merged
merged 8 commits into from Aug 9, 2023

Conversation

Zhenyun20023
Copy link
Contributor

@Zhenyun20023 Zhenyun20023 commented Aug 8, 2023

feature
Currently QuerySummary only exposes avg value.
Exposing percentile values (e.g. P99) is useful.
Example usage: after running queries, we can decide whether the query latency is acceptable or not based on high percentile values.

@Zhenyun20023 Zhenyun20023 changed the title Zen 2023 08 query summary Zen 2023 08 query summary exposing percentiles Aug 8, 2023
@Zhenyun20023
Copy link
Contributor Author

Zhenyun20023 commented Aug 8, 2023

label: can be "feature" or "enhancement".
Currently QuerySummary only exposes avg value.
We 'd like to get percentile values (e.g. P99). Example usage: after running queries, we can decide whether the query latency is acceptable or not based on high percentile values.

@Zhenyun20023 Zhenyun20023 marked this pull request as ready for review August 8, 2023 22:01
@Zhenyun20023
Copy link
Contributor Author

@klsince can be the reviewer (he has the context).

@Zhenyun20023 Zhenyun20023 changed the title Zen 2023 08 query summary exposing percentiles QuerySummary exposing percentiles Aug 8, 2023
Copy link
Contributor

@klsince klsince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a comment on code format, otherwise LGTM

@@ -877,6 +877,10 @@ public void addValue(double value) {
}
}

public double getPercentile(double p){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public double getPercentile(double p){
public double getPercentile(double p) {

@@ -944,6 +948,15 @@ public double getAvgClientTime() {
return _avgClientTime;
}

public double getPercentile(double p){
if(_statisticsList == null || _statisticsList.size() == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(_statisticsList == null || _statisticsList.size() == 0)
if (_statisticsList == null || _statisticsList.size() == 0) {
return 0.0;
}

@@ -944,6 +948,16 @@ public double getAvgClientTime() {
return _avgClientTime;
}

public double getPercentile(double p) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we don't need this method, as we have this method below, and we can get any percentile we'd need from the list of stats.

public List<Statistics> getStatisticsList() {
      return _statisticsList;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Statistics class is a private one.
Having the public getPercentile() can allow external usage of the percentile stats.

@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2023

Codecov Report

Merging #11299 (793507f) into master (d7d3bdb) will not change coverage.
Report is 2 commits behind head on master.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11299   +/-   ##
=======================================
  Coverage    0.11%    0.11%           
=======================================
  Files        2230     2230           
  Lines      120140   120140           
  Branches    18218    18218           
=======================================
  Hits          137      137           
  Misses     119983   119983           
  Partials       20       20           
Flag Coverage Δ
integration1temurin11 0.00% <ø> (ø)
integration1temurin17 0.00% <ø> (ø)
integration1temurin20 0.00% <ø> (ø)
integration2temurin11 0.00% <ø> (ø)
integration2temurin17 0.00% <ø> (ø)
integration2temurin20 0.00% <ø> (ø)
unittests1temurin11 0.00% <ø> (ø)
unittests1temurin17 0.00% <ø> (ø)
unittests1temurin20 0.00% <ø> (ø)
unittests2temurin11 0.11% <ø> (ø)
unittests2temurin17 0.11% <ø> (ø)
unittests2temurin20 0.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Zhenyun20023 Zhenyun20023 changed the title QuerySummary exposing percentiles Add percentiles report in QuerySummary Aug 8, 2023
@klsince klsince merged commit 95f2382 into apache:master Aug 9, 2023
22 checks passed
@Zhenyun20023 Zhenyun20023 deleted the zen-2023-08-QuerySummary branch August 9, 2023 15:07
s0nskar pushed a commit to s0nskar/pinot that referenced this pull request Aug 10, 2023
* Exposing percentile values of the runs

---------

Co-authored-by: zhenyun <test@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants