[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. IMPALA-8560: Prometheus metrics support in Impala -- This change adds Prometheus text explosion format metric generation. -- More details can be found below: -- https://prometheus.io/docs/instrumenting/exposition_formats -- Added unit test to test this change Tests: -- Feed all this metrics to prometheus running on local host -- Also ran it against a "./promtool" to check for any error in metrics format for prometheus. Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 --- M be/src/util/collection-metrics.h M be/src/util/histogram-metric.h M be/src/util/metrics-test.cc M be/src/util/metrics.cc M be/src/util/metrics.h M tests/webserver/test_web_pages.py 6 files changed, 681 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/13345/10 -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 10 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has posted comments on this change. ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. Patch Set 9: I have added py tests -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 9 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 29 May 2019 02:15:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has posted comments on this change. (
http://gerrit.cloudera.org:8080/13345 )
Change subject: IMPALA-8560: Prometheus metrics support in Impala
..
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13345/7/be/src/util/metrics-test.cc
File be/src/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/13345/7/be/src/util/metrics-test.cc@588
PS7, Line 588: TEST_F(MetricsTest, StatsMetricsPrometheus) {
> Why did you remove these tests?
Yeah, thanks for checking about this.
I removed it because code is ignoring 'property' metrics.
Since I saw some property metric were causing problems when passed to
prometheus. Code used to convert property metric to gauge when passing to
prometheus but gauge cannot have string values.
--
To view, visit http://gerrit.cloudera.org:8080/13345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08
Gerrit-Change-Number: 13345
Gerrit-PatchSet: 8
Gerrit-Owner: Harshil
Gerrit-Reviewer: Harshil
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 28 May 2019 23:19:56 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. IMPALA-8560: Prometheus metrics support in Impala -- This change adds Prometheus text explosion format metric generation. -- More details can be found below: -- https://prometheus.io/docs/instrumenting/exposition_formats -- Added unit test to test this change Tests: -- Feed all this metrics to prometheus running on local host -- Also ran it against a "./promtool" to check for any error in metrics format for prometheus. Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 --- M be/src/util/collection-metrics.h M be/src/util/histogram-metric.h M be/src/util/metrics-test.cc M be/src/util/metrics.cc M be/src/util/metrics.h 5 files changed, 673 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/13345/8 -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 8 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has posted comments on this change. (
http://gerrit.cloudera.org:8080/13345 )
Change subject: IMPALA-8560: Prometheus metrics support in Impala
..
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13345/7/be/src/util/metrics-test.cc
File be/src/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/13345/7/be/src/util/metrics-test.cc@472
PS7, Line 472: if (std::string(name) == "stats_metric" || std::string(name)
== "histogram_metric") {
> std::string() not needed.
Done
http://gerrit.cloudera.org:8080/#/c/13345/7/be/src/util/metrics.cc
File be/src/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/13345/7/be/src/util/metrics.cc@181
PS7, Line 181: std:
> no std:: needed
Done
--
To view, visit http://gerrit.cloudera.org:8080/13345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08
Gerrit-Change-Number: 13345
Gerrit-PatchSet: 7
Gerrit-Owner: Harshil
Gerrit-Reviewer: Harshil
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 28 May 2019 20:38:00 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. IMPALA-8560: Prometheus metrics support in Impala -- This change adds Prometheus text explosion format metric generation. -- More details can be found below: -- https://prometheus.io/docs/instrumenting/exposition_formats -- Added unit test to test this change Tests: -- Feed all this metrics to prometheus running on local host -- Also ran it against a "./promtool" to check for any error in metrics format for prometheus. Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 --- M be/src/util/collection-metrics.h M be/src/util/histogram-metric.h M be/src/util/metrics-test.cc M be/src/util/metrics.cc M be/src/util/metrics.h 5 files changed, 689 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/13345/7 -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 7 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has posted comments on this change. ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/13345/6/be/src/util/collection-metrics.h File be/src/util/collection-metrics.h: http://gerrit.cloudera.org:8080/#/c/13345/6/be/src/util/collection-metrics.h@184 PS6, Line 184: static_cast(boost::accumulators::min(acc_)), unit_) > These casts all seem unnecessary - ConvertToPrometheusSecs should work for Done http://gerrit.cloudera.org:8080/#/c/13345/6/be/src/util/metrics.h File be/src/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13345/6/be/src/util/metrics.h@116 PS6, Line 116: inline > Inline not needed. Done http://gerrit.cloudera.org:8080/#/c/13345/6/be/src/util/metrics.h@117 PS6, Line 117: if (type == TUnit::type::TIME_MS || type == TUnit::type::TIME_US > if isn't needed - just Done http://gerrit.cloudera.org:8080/#/c/13345/5/be/src/util/metrics.h File be/src/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13345/5/be/src/util/metrics.h@101 PS5, Line 101: /// https://github.com/prometheus/docs/blob/master/content/docs/instrumenting/ > line too long (101 > 90) Done http://gerrit.cloudera.org:8080/#/c/13345/5/be/src/util/metrics.h@112 PS5, Line 112: > ok thanks Tim. Done http://gerrit.cloudera.org:8080/#/c/13345/5/be/src/util/metrics.h@191 PS5, Line 191: , TUnit > TIME_MS Done -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 6 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 28 May 2019 17:14:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. IMPALA-8560: Prometheus metrics support in Impala -- This change adds Prometheus text explosion format metric generation. -- More details can be found below: -- https://prometheus.io/docs/instrumenting/exposition_formats -- Added unit test to test this change Testing Done: -- Feed all this metrics to prometheus running on local host -- Also ran it against a "./promtool" to check for any error in metrics format for prometheus. Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 --- M be/src/util/collection-metrics.h M be/src/util/histogram-metric.h M be/src/util/metrics-test.cc M be/src/util/metrics.cc M be/src/util/metrics.h 5 files changed, 696 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/13345/6 -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 6 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has posted comments on this change. (
http://gerrit.cloudera.org:8080/13345 )
Change subject: IMPALA-8560: Prometheus metrics support in Impala
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13345/5/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/13345/5/be/src/util/metrics.h@112
PS5, Line 112: std::string ConvertToPrometheusSecs(std::stringstream* val,
TUnit::type unit) {
> I have two issues with the function/pattern:
ok thanks Tim.
I think I can start with your change and go ahead update other than scalar
metric and see how it goes.
--
To view, visit http://gerrit.cloudera.org:8080/13345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08
Gerrit-Change-Number: 13345
Gerrit-PatchSet: 5
Gerrit-Owner: Harshil
Gerrit-Reviewer: Harshil
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 23 May 2019 00:21:49 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. IMPALA-8560: Prometheus metrics support in Impala -- This change adds Prometheus text explosion format metric generation. -- More details can be found below: -- https://prometheus.io/docs/instrumenting/exposition_formats -- Added unit test to test this change Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 --- M be/src/util/collection-metrics.h M be/src/util/histogram-metric.h M be/src/util/metrics-test.cc M be/src/util/metrics.cc M be/src/util/metrics.h 5 files changed, 653 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/13345/5 -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 5 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has posted comments on this change. ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/13345/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13345/4//COMMIT_MSG@9 PS4, Line 9: -- This change adds Prometheus text explosion format metric generation. > My reading on the time units is that the prometheus convention is to conver Done http://gerrit.cloudera.org:8080/#/c/13345/4//COMMIT_MSG@13 PS4, Line 13: > Did you try feeding the output of the metrics page into prometheus? I think I have local prometheus running on same node that is ingesting this metrics and verified it with latest change. Also ran it against a "./promtool" to check for any error in metrics format for prometheus. http://gerrit.cloudera.org:8080/#/c/13345/4/be/src/util/metrics.h File be/src/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13345/4/be/src/util/metrics.h@102 PS4, Line 102: /// name, value, human_readable, description > Should have asked first time around, but can you include a link to https:// Done http://gerrit.cloudera.org:8080/#/c/13345/4/be/src/util/metrics.cc File be/src/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/13345/4/be/src/util/metrics.cc@97 PS4, Line 97: metricsPrometheus > metrics_prometheus to follow the same convention as other endpoints like lo Done -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 4 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 21 May 2019 23:38:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has posted comments on this change. ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/13345/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13345/4//COMMIT_MSG@9 PS4, Line 9: -- This change adds Prometheus text explosion format metric generation. > I looked more at So for 3) there is something called job_name in prometheus config, that get appended to metric, below has details https://prometheus.io/docs/prometheus/latest/configuration/configuration/#scrape_config For 2) there is no difference in prometheus perspective between S/MS/NS, there is only float or int. Can we convert all time values to either second or millisecond ? Below is based on doc: """ value is a float represented as required by Go's ParseFloat() function. In addition to standard numerical values, Nan, +Inf, and -Inf are valid values representing not a number, positive infinity, and negative infinity, respectively. The timestamp is an int64 (milliseconds since epoch, i.e. 1970-01-01 00:00:00 UTC, excluding leap seconds), represented as required by Go's ParseInt() function. """ For 1) I will address it. -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 4 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 18 May 2019 00:39:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. IMPALA-8560: Prometheus metrics support in Impala -- This change adds Prometheus text explosion format metric generation. -- More details can be found below: -- https://prometheus.io/docs/instrumenting/exposition_formats -- Added unit test to test this change Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 --- M be/src/util/collection-metrics.h M be/src/util/histogram-metric.h M be/src/util/metrics-test.cc M be/src/util/metrics.cc M be/src/util/metrics.h 5 files changed, 550 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/13345/4 -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 4 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. IMPALA-8560: Prometheus metrics support in Impala -- This change adds Prometheus text explosion format metric generation. -- More details can be found below: -- https://prometheus.io/docs/instrumenting/exposition_formats -- Added unit test to test this change Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 --- M be/src/util/collection-metrics.h M be/src/util/histogram-metric.h M be/src/util/metrics-test.cc M be/src/util/metrics.cc M be/src/util/metrics.h 5 files changed, 548 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/13345/3 -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 3 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has posted comments on this change. (
http://gerrit.cloudera.org:8080/13345 )
Change subject: IMPALA-8560: Prometheus metrics support in Impala
..
Patch Set 2:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/collection-metrics.h
File be/src/util/collection-metrics.h:
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/collection-metrics.h@80
PS2, Line 80: +
> <<
Done
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/collection-metrics.h@170
PS2, Line 170: string
> nit: std::string
Done
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/collection-metrics.h@173
PS2, Line 173: *val << name;
> You can combine these lines by chaining <<. If you do that here and below i
Done
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/histogram-metric.h
File be/src/util/histogram-metric.h:
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/histogram-metric.h@75
PS2, Line 75: string
> nit: std::string
Done
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/histogram-metric.h@80
PS2, Line 80: *value << "{le=0.2} ";
> Same comment about chaining << to get this into fewer lines. If you look at
Done
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/metrics-test.cc
File be/src/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/metrics-test.cc@470
PS2, Line 470: +
> nit: << instead of + here and in below lines
Done
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/metrics-test.cc@488
PS2, Line 488: AddMetricDef("gauge", TMetricKind::GAUGE, TUnit::NONE);
> Can you add tests for the supported unit types:
Done
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/metrics-test.cc@495
PS2, Line 495: TEST_F(MetricsTest, PropertiesPrometheus) {
> Can you test some more edge cases with the strings, e.g. what happens if th
Done
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/metrics-test.cc@513
PS2, Line 513: AssertPrometheus(stats_val, "stats_metric",
> Do we have test coverage for all the floating point formats in the spec: ht
I am not sure if I found a specific list of floating points that prometheus
support. Based on FAQ, all sample values are 64 bit floats.
https://prometheus.io/docs/introduction/faq/#why-are-all-sample-values-64-bit-floats-i-want-integers
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/metrics-test.cc@514
PS2, Line 514: "stats_metric_count 2\nstats_metric_last
20.00\nstats_metric_min "
> Can you format this in a way that makes it easier to see the lines, i.e.
Done
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/metrics-test.cc@533
PS2, Line 533: "histogram-metric{le=0.2} 2500\nhistogram-metric{le=0.5} "
> Same comment on formatting.
Done
http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/metrics-test.cc@543
PS2, Line 543: exp_val << "# HELP description\n# TYPE counter1
COUNTER\ncounter1 2048\n# HELP "
> same comment on formatting.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08
Gerrit-Change-Number: 13345
Gerrit-PatchSet: 2
Gerrit-Owner: Harshil
Gerrit-Reviewer: Harshil
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Fri, 17 May 2019 23:30:01 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8560: Prometheus metrics support in Impala
Harshil has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/13345 ) Change subject: IMPALA-8560: Prometheus metrics support in Impala .. IMPALA-8560: Prometheus metrics support in Impala -- This change adds Prometheus text explosion format metric generation. -- More details can be found below: -- https://prometheus.io/docs/instrumenting/exposition_formats -- Added unit test to test this change Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 --- M be/src/util/collection-metrics.h M be/src/util/histogram-metric.h M be/src/util/metrics-test.cc M be/src/util/metrics.cc M be/src/util/metrics.h 5 files changed, 280 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/13345/2 -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 2 Gerrit-Owner: Harshil Gerrit-Reviewer: Harshil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] DWX-124: Prometheus metrics support in Impala
Harshil has posted comments on this change. (
http://gerrit.cloudera.org:8080/13345 )
Change subject: DWX-124: Prometheus metrics support in Impala
..
Patch Set 1:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/13345/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13345/1//COMMIT_MSG@9
PS1, Line 9: -- This change adds Prometheus text explosion format metric
generation in impala, more details
> Can you wrap lines to 72 chars in commit messages.
Done
http://gerrit.cloudera.org:8080/#/c/13345/1//COMMIT_MSG@13
PS1, Line 13: -- Doc that talk about this approach is here:
https://docs.google.com/document/d/1KeOuFowH83q9l7iGHFMXL2mAejt4EZCjn3XfKDu7sho/edit?usp=sharing
> This doc isn't publicly accesssible.
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h
File be/src/util/collection-metrics.h:
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@170
PS1, Line 170: string
> Use std::string in header files (we have a policy not to import std::string
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@170
PS1, Line 170: string name, std::stringstream* val, std::stringstream*
metric_kind) {
> It's annoying that the original author of this file didn't move the ToJson(
ok thanks
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@172
PS1, Line 172: string collect_data;
> We should just append to val instead of adding an intermediate string.
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@175
PS1, Line 175: tatic_cast(
> Why are the casts needed? this seems suspicious to me - like it might not w
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@178
PS1, Line 178: collect_data += name + "_last " + std::to_string(value_) +
"\n";
> For string concatenation we either prefer using << to append to a stringstr
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@200
PS1, Line 200: sqrt
> std::sqrt
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@204
PS1, Line 204: *metric_kind << "# TYPE " + name + " counter";
> use << to append to the stream instead of string concatenation.
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/histogram-metric.h
File be/src/util/histogram-metric.h:
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/histogram-metric.h@81
PS1, Line 81: histogram_data +=
> Same comments about using << to append directly to value
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/histogram-metric.h@99
PS1, Line 99: *metric_kind << "# TYPE " + name + " histogram";
> Same comment about <<
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.h@171
PS1, Line 171: string
> std::string in headers
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.h@173
PS1, Line 173: *metric_kind << "# TYPE " + name + " " +
PrintThriftEnum(kind()).c_str();
> Use << instead of +
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc
File be/src/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@190
PS1, Line 190: return;
> return not needed
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@206
PS1, Line 206:
> Don't need to add vertical whitespace
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@220
PS1, Line 220: void MetricGroup::ToPrometheus(bool include_children,
std::stringstream* out_val) {
> Same comments apply about using << instead of strings.
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@220
PS1, Line 220: std::
> shouldn't need to use std:: prefix in .cc files.
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@245
PS1, Line 245:
> unnecessary vertical whitespace and return
Done
http://gerrit.cloudera.org:8080/#/c/13345/1/bin/distcc/distcc_server_setup.sh
File bin/distcc/distcc_server_setup.sh:
http://gerrit.cloudera.org:8080/#/c/13345/1/bin/distcc/distcc_server_setup.sh@58
PS1, Line 58: if ! [[ $LSB_VERSION == 14.04 || $LSB_VERSION == 16.04 ||
$LSB_VERSION == 18.04 ]]; then
> Great if this works - but can you do this in a separate change? I also want
I am using ubuntu 18.04, so I tested this by running it on host that I have.
--
To view, visit http://gerrit.cloudera.org:8080/13345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08
Gerrit-Change-Number: 13345
Gerrit-PatchSet: 1
Gerrit-Owner: Harshil
Gerrit-Reviewer: Harshil
Gerrit-Reviewer: Impala Public Jenkins
