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.000000\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 <hars...@cloudera.com> Gerrit-Reviewer: Harshil <hars...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 17 May 2019 23:30:01 +0000 Gerrit-HasComments: Yes
