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 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/13345/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13345/4//COMMIT_MSG@13 PS4, Line 13: > I have local prometheus running on same node that is ingesting this metrics Can you add a "Testing" section to the commit message? That'll make it more visible for others. 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: * The name of the function is confusing, since it's called for all metrics, including those that don't need conversion and aren't time types. I think it should be something that makes it more obvious at the callsite why it's called and what it will do (I'd suggest a name but I don't think we've decided on the behaviour yet). * Converting to a string and back again is really awkward and hard to reason about. I'm guessing this was a workaround for C++ template hell around T, because T can be std::string which doesn't support a / operation. We can solve the template thing in a cleaner way by having multiple implementations of a templated function, with the non-numeric ones just no-ops (since we don't expect the conversion to be done for non-numeric metrics). As a proof of concept I got it working for one metric - see https://github.com/timarmstrong/impala/tree/prometheus http://gerrit.cloudera.org:8080/#/c/13345/5/be/src/util/metrics.h@191 PS5, Line 191: TIMS_MS TIME_MS -- 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 <[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: Wed, 22 May 2019 20:44:11 +0000 Gerrit-HasComments: Yes
