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 4: (5 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. I looked more at https://prometheus.io/docs/practices/naming/ and I realised that we aren't following best practices. I have a few concerns to tackle either in this or a follow-on patch. 1. Our metric names are invalid because they include a period and hyphens: https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels . I think this needs to be tackled now since I think it probably means prometheus can't consume the metrics. 2. We're not indicating the units in any of the metrics. This limits the usefulness, and is particularly confusing for time where we have S/MS/NS. I think this is critical to address. 3. metrics aren't prefixed with anything that would namespace them to impala. I don't know if this is a major problem in prometheus (i.e. they become unusable) or if it's minor. We could consider prefixing everything with impala or apache_impala automatically. I think we need to fix #1 before committing. It seems like converting . and - to _ automatically would be sufficient. #2 and #3 also need to be thought about and addressed before we can call the prometheus support complete. 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 that would be useful validation that we got the format correct. 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@513 PS2, Line 513: MetricGroup metrics("CounterMetrics"); > I am not sure if I found a specific list of floating points that prometheus It mentions that it has to be parsable by go's parsefloat function: https://github.com/prometheus/docs/blob/master/content/docs/instrumenting/exposition_formats.md . Would be good to test edge cases like NaN, +Inf, -Inf. 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://github.com/prometheus/docs/blob/master/content/docs/instrumenting/exposition_formats.md to make it easier to find for people maintaining the code. 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 log_level (we don't use camel case elsewhere) -- 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 <[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: Sat, 18 May 2019 00:22:26 +0000 Gerrit-HasComments: Yes
