Harshil has posted comments on this change. ( http://gerrit.cloudera.org:8080/13345 )
Change subject: DWX-124: Prometheus metrics support in Impala ...................................................................... Patch Set 1: (19 comments) http://gerrit.cloudera.org:8080/#/c/13345/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13345/1//COMMIT_MSG@9 PS1, Line 9: -- This change adds Prometheus text explosion format metric generation in impala, more details > Can you wrap lines to 72 chars in commit messages. Done http://gerrit.cloudera.org:8080/#/c/13345/1//COMMIT_MSG@13 PS1, Line 13: -- Doc that talk about this approach is here: https://docs.google.com/document/d/1KeOuFowH83q9l7iGHFMXL2mAejt4EZCjn3XfKDu7sho/edit?usp=sharing > This doc isn't publicly accesssible. Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h File be/src/util/collection-metrics.h: http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@170 PS1, Line 170: string > Use std::string in header files (we have a policy not to import std::string Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@170 PS1, Line 170: string name, std::stringstream* val, std::stringstream* metric_kind) { > It's annoying that the original author of this file didn't move the ToJson( ok thanks http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@172 PS1, Line 172: string collect_data; > We should just append to val instead of adding an intermediate string. Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@175 PS1, Line 175: tatic_cast<uint64_t>( > Why are the casts needed? this seems suspicious to me - like it might not w Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@178 PS1, Line 178: collect_data += name + "_last " + std::to_string(value_) + "\n"; > For string concatenation we either prefer using << to append to a stringstr Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@200 PS1, Line 200: sqrt > std::sqrt Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@204 PS1, Line 204: *metric_kind << "# TYPE " + name + " counter"; > use << to append to the stream instead of string concatenation. Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/histogram-metric.h File be/src/util/histogram-metric.h: http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/histogram-metric.h@81 PS1, Line 81: histogram_data += > Same comments about using << to append directly to value Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/histogram-metric.h@99 PS1, Line 99: *metric_kind << "# TYPE " + name + " histogram"; > Same comment about << Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.h File be/src/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.h@171 PS1, Line 171: string > std::string in headers Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.h@173 PS1, Line 173: *metric_kind << "# TYPE " + name + " " + PrintThriftEnum(kind()).c_str(); > Use << instead of + Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc File be/src/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@190 PS1, Line 190: return; > return not needed Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@206 PS1, Line 206: > Don't need to add vertical whitespace Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@220 PS1, Line 220: void MetricGroup::ToPrometheus(bool include_children, std::stringstream* out_val) { > Same comments apply about using << instead of strings. Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@220 PS1, Line 220: std:: > shouldn't need to use std:: prefix in .cc files. Done http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@245 PS1, Line 245: > unnecessary vertical whitespace and return Done http://gerrit.cloudera.org:8080/#/c/13345/1/bin/distcc/distcc_server_setup.sh File bin/distcc/distcc_server_setup.sh: http://gerrit.cloudera.org:8080/#/c/13345/1/bin/distcc/distcc_server_setup.sh@58 PS1, Line 58: if ! [[ $LSB_VERSION == 14.04 || $LSB_VERSION == 16.04 || $LSB_VERSION == 18.04 ]]; then > Great if this works - but can you do this in a separate change? I also want I am using ubuntu 18.04, so I tested this by running it on host that I have. -- 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: 1 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: Thu, 16 May 2019 19:06:25 +0000 Gerrit-HasComments: Yes
