Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13345 )
Change subject: IMPALA-8560: Prometheus metrics support in Impala ...................................................................... Patch Set 2: (13 comments) Some more style nits and I some requests for additional test coverage. You probably have a better understanding of the prometheus format than I do, so if you can think of more edge cases that I didn't ask for tests for, please add them. 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: + << You can also just combine these lines, i.e. *val << name << " " << s; http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/collection-metrics.h@170 PS2, Line 170: string nit: std::string 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 it will be a lot more concise. 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 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 some of the LOG statements in the code we even have multi-line chains in a lot of places. 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 http://gerrit.cloudera.org:8080/#/c/13345/2/be/src/util/metrics-test.cc@477 PS2, Line 477: TEST_F(MetricsTest, CountersPrometheus) { Can you add a BYTE unit counter? 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: $ grep unit common/thrift/metrics.json | sort | uniq "units": "BYTES", "units": "NONE", "units": "TIME_MS", "units": "TIME_NS", "units": "TIME_S", "units": "UNIT", 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 there's a line break in the string? It looks like the prometheus format requires strings to be escaped: https://github.com/prometheus/docs/blob/master/content/docs/instrumenting/exposition_formats.md 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: https://github.com/prometheus/docs/blob/master/content/docs/instrumenting/exposition_formats.md 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.000000\nstats_metric_min " Can you format this in a way that makes it easier to see the lines, i.e. "stats_metric_count 2\n" "stats_metric_last 20.000000\n" "stats_metric_min 10\n" ... 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. 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. -- 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 <[email protected]> Gerrit-Reviewer: Harshil <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 16 May 2019 20:57:28 +0000 Gerrit-HasComments: Yes
