You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2019/09/23 18:26:53 UTC

[Impala-ASF-CR] IMPALA-8946: Fix histogram rendering to Prometheus 

Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14280


Change subject: IMPALA-8946: Fix histogram rendering to Prometheus
......................................................................

IMPALA-8946: Fix histogram rendering to Prometheus

Prometheus has two formats that allows to calculate quantiles (on client
side): Histograms and Summaries. The former requires to pick a set of
buckets, while the latter only requires to specifiy which quantiles
(percentiles) the application wants to provide. The difference between
summaries and histograms on Prometheus can be found here:
https://prometheus.io/docs/practices/histograms/

Previous to this changes, Impala histograms were rendered as Prometheus
histograms, but it was calculating percentiles instead of bucketing the
recorded values, which is not what Prometheus clients expects.

From now on, Impala histograms will be rendered as Prometheus summaries
instead, which expects quantiles.

Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
---
M be/src/util/histogram-metric.cc
M be/src/util/metrics-test.cc
2 files changed, 53 insertions(+), 71 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/80/14280/1
-- 
To view, visit http://gerrit.cloudera.org:8080/14280
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Gerrit-Change-Number: 14280
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8946: Fix histogram rendering to Prometheus

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14280 )

Change subject: IMPALA-8946: Fix histogram rendering to Prometheus
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4615/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/14280
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Gerrit-Change-Number: 14280
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Sep 2019 19:07:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8946: Fix histogram rendering to Prometheus

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14280 )

Change subject: IMPALA-8946: Fix histogram rendering to Prometheus
......................................................................


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5037/ DRY_RUN=true


-- 
To view, visit http://gerrit.cloudera.org:8080/14280
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Gerrit-Change-Number: 14280
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 01 Oct 2019 19:04:19 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8946: Fix histogram rendering to Prometheus

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14280 )

Change subject: IMPALA-8946: Fix histogram rendering to Prometheus
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4690/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/14280
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Gerrit-Change-Number: 14280
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 01 Oct 2019 18:52:24 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8946: Fix histogram rendering to Prometheus

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14280 )

Change subject: IMPALA-8946: Fix histogram rendering to Prometheus
......................................................................


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5034/ DRY_RUN=true


-- 
To view, visit http://gerrit.cloudera.org:8080/14280
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Gerrit-Change-Number: 14280
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 01 Oct 2019 18:11:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8946: Fix histogram rendering to Prometheus

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14280 )

Change subject: IMPALA-8946: Fix histogram rendering to Prometheus
......................................................................


Patch Set 2: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/14280
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Gerrit-Change-Number: 14280
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 01 Oct 2019 23:03:35 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8946: Fix histogram rendering to Prometheus

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14280 )

Change subject: IMPALA-8946: Fix histogram rendering to Prometheus
......................................................................

IMPALA-8946: Fix histogram rendering to Prometheus

Prometheus has two formats that allows to calculate quantiles (on client
side): Histograms and Summaries. The former requires to pick a set of
buckets, while the latter only requires to specifiy which quantiles
(percentiles) the application wants to provide. The difference between
summaries and histograms on Prometheus can be found here:
https://prometheus.io/docs/practices/histograms/

Previous to this changes, Impala histograms were rendered as Prometheus
histograms, but it was calculating percentiles instead of bucketing the
recorded values, which is not what Prometheus clients expects.

From now on, Impala histograms will be rendered as Prometheus summaries
instead, which expects quantiles.

To be compliant with Prometheus summaries, a `_sum` serie needs to
be added, containing the sum of all the registered values.

The current `HdrHistogram` implementation on be::utils does not have
the `TotalSum` method (and the required properties to track it). I
copied the implementation from be::kudu::utils and added a couple of
unit tests which covers the sum method.

Also adapted the Prometheus tests to verify that sum serie is properly
rendered.

Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Reviewed-on: http://gerrit.cloudera.org:8080/14280
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
---
M be/src/util/CMakeLists.txt
A be/src/util/hdr-histogram-test.cc
M be/src/util/hdr-histogram.cc
M be/src/util/hdr-histogram.h
M be/src/util/histogram-metric.cc
M be/src/util/metrics-test.cc
6 files changed, 127 insertions(+), 75 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Tim Armstrong: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/14280
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Gerrit-Change-Number: 14280
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8946: Fix histogram rendering to Prometheus

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14280

to look at the new patch set (#2).

Change subject: IMPALA-8946: Fix histogram rendering to Prometheus
......................................................................

IMPALA-8946: Fix histogram rendering to Prometheus

Prometheus has two formats that allows to calculate quantiles (on client
side): Histograms and Summaries. The former requires to pick a set of
buckets, while the latter only requires to specifiy which quantiles
(percentiles) the application wants to provide. The difference between
summaries and histograms on Prometheus can be found here:
https://prometheus.io/docs/practices/histograms/

Previous to this changes, Impala histograms were rendered as Prometheus
histograms, but it was calculating percentiles instead of bucketing the
recorded values, which is not what Prometheus clients expects.

From now on, Impala histograms will be rendered as Prometheus summaries
instead, which expects quantiles.

To be compliant with Prometheus summaries, a `_sum` serie needs to
be added, containing the sum of all the registered values.

The current `HdrHistogram` implementation on be::utils does not have
the `TotalSum` method (and the required properties to track it). I
copied the implementation from be::kudu::utils and added a couple of
unit tests which covers the sum method.

Also adapted the Prometheus tests to verify that sum serie is properly
rendered.

Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
---
M be/src/util/CMakeLists.txt
A be/src/util/hdr-histogram-test.cc
M be/src/util/hdr-histogram.cc
M be/src/util/hdr-histogram.h
M be/src/util/histogram-metric.cc
M be/src/util/metrics-test.cc
6 files changed, 127 insertions(+), 75 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/80/14280/2
-- 
To view, visit http://gerrit.cloudera.org:8080/14280
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Gerrit-Change-Number: 14280
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8946: Fix histogram rendering to Prometheus

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14280 )

Change subject: IMPALA-8946: Fix histogram rendering to Prometheus
......................................................................


Patch Set 2: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/14280
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Gerrit-Change-Number: 14280
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Oct 2019 01:11:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8946: Fix histogram rendering to Prometheus

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14280 )

Change subject: IMPALA-8946: Fix histogram rendering to Prometheus
......................................................................


Patch Set 1:

(1 comment)

Thank you for noticing and fixing this! I think this looks good, just one question about whether we want to fix an additional issue as part of this.

http://gerrit.cloudera.org:8080/#/c/14280/1/be/src/util/histogram-metric.cc
File be/src/util/histogram-metric.cc:

http://gerrit.cloudera.org:8080/#/c/14280/1/be/src/util/histogram-metric.cc@131
PS1, Line 131:     *value << name << "_count " << histogram_->TotalCount();
It looks like prometheus also wants a _sum member for summary metrics? I'm not sure how gracefully it handles it being missing. Obviously your change makes this much closer to the spec. It seems like we could extend HistogramMetric/HdrHistogram to track the sum as well.



-- 
To view, visit http://gerrit.cloudera.org:8080/14280
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Gerrit-Change-Number: 14280
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Sep 2019 18:41:18 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8946: Fix histogram rendering to Prometheus

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14280 )

Change subject: IMPALA-8946: Fix histogram rendering to Prometheus
......................................................................


Patch Set 1:

Guillem didn't want to give gerrit access to his github account, so we're doing the review partially here and partially on JIRA: IMPALA-8946


-- 
To view, visit http://gerrit.cloudera.org:8080/14280
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8849586964140c040ca4ea14b777c2d98d126b59
Gerrit-Change-Number: 14280
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Sep 2019 18:42:16 +0000
Gerrit-HasComments: No