Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13941 )
Change subject: IMPALA-8799: prefix Prometheus metrics with "impala_" ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/13941/2/be/src/util/metrics-test.cc File be/src/util/metrics-test.cc: http://gerrit.cloudera.org:8080/#/c/13941/2/be/src/util/metrics-test.cc@467 PS2, Line 467: TEST_F(MetricsTest, PrometheusMetricNames) { > nit: could add a test for "an_impala_metric" and one with uppercase charact Done http://gerrit.cloudera.org:8080/#/c/13941/2/be/src/util/metrics.h File be/src/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13941/2/be/src/util/metrics.h@472 PS2, Line 472: impala > I wonder if we should actually test for "impala_" here to make sure that al I started off doing that, but I was checking before other transformations so "impala-server.metric" became impala_impala_server_metric. I added some examples to illustrate the transformation in a few cases, I think that makes it a little clearer why it is doing what it does. I could alternatively change the implementation so that it checked if "impala_" was the prefix after it did the character substitutions. I was trying to reduce the number of string reallocations required. I also realise that I should have made some comments in the body explaining what I was trying to do there. So I'm not really attached to the current solution, but it seemed adequate. http://gerrit.cloudera.org:8080/#/c/13941/2/be/src/util/metrics.cc File be/src/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/13941/2/be/src/util/metrics.cc@310 PS2, Line 310: LOG(INFO) > Should this be something like VLOG(3)? Otherwise I think we log this every Oops, leftover debugging code. Just deleted it. -- To view, visit http://gerrit.cloudera.org:8080/13941 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I607ee2a456c3ec2380ca996080ae497302b5b44f Gerrit-Change-Number: 13941 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Sun, 28 Jul 2019 19:57:31 +0000 Gerrit-HasComments: Yes
