Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/19133 )
Change subject: [metrics] Expose Prometheus Metrics in Kudu ...................................................................... Patch Set 9: (14 comments) http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/server/default_path_handlers.cc@544 PS6, Line 544: constexpr bool not_styled = false; : constexpr bool is_on_nav_b > nit: constexpr might be even better since it opens more opportunities for c Done http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/server/server_base.cc@972 PS7, Line 972: , > nit: add a space after comma done http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics-test.cc File src/kudu/util/metrics-test.cc: http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics-test.cc@308 PS7, Line 308: ASSERT_EQ(expected_output, output.str()); > nit for here and below: the reference/expected value should come first, oth done http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics-test.cc@403 PS7, Line 403: const string expected_outpu > nit for here and other places: consider adding 'const' to be more explicit done http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.h@586 PS7, Line 586: & > style nit: stick the ampersand to the type done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@75 PS6, Line 75: > It's something like Done, thanks for clarifying http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@413 PS6, Line 413: const string master_server = "kudu.master"; > Alright, but then why to store the status of the GetMetricsAndAttrs() call Re added the status check for NotFound() to skip over filtered entities. If there are any extra cases that also should be considered that come to your mind, please do share http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@983 PS6, Line 983: prefix, prototype_->name(), "_sum", : MetricUnit::Name(prototype_->unit()), total_sum()); : : writer->WriteEntry(output); : } : : // : // Counter : // : // This implementation is optimized by using a striped counter. See LongAdder for details. : : scoped_refptr<Counter> CounterPrototype::Instantiate(const scoped_refptr<MetricEntity>& entity) { : return entity->FindOrCreateCounter(this); : } : > Cool, thanks for clarifying on this. Can't find an explicit location on where it is mentioned in the documentation but I have seen use cases, and they can be either ignored or utilized by the users. This explanation itself is provided in the commit with the text explosion format. http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@417 PS7, Line 417: tMetricsAndAt > nit: consider defining a constant for this and for "kudu.tabletserver" as w done http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@731 PS7, Line 731: writer->String(description()); : : writer->String("level"); : writer->String(MetricLevelName(level())); : } : } : > nit for here and below: could get rid of temporary variable and the assignm Done http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@900 PS7, Line 900: > nit for here and elsewher: drop the 'strings::' namespace prefix since this Done http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@974 PS7, Line 974: void MeanGauge::WriteValue(PrometheusWriter* writer, const std::string& prefix) const { : auto output = Substitute("$0$1{unit_type=\"$2\"} $3\n", prefix, prototype_->name(), : > nit: could join these into Done http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@1100 PS7, Line 1100: : output = Substitute("$0$1$2{unit_type=\"$3\", le=\"0.75\"} $4\n", : prefix, prototype_->name(), "_buck > nit: please add a comment to explain that the snapshot is taken to preserve done http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@1109 PS7, Line 1109: snapsho > nit: remove the 'strings::' namespace prefix and add 'using strings::Substi Done -- To view, visit http://gerrit.cloudera.org:8080/19133 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739 Gerrit-Change-Number: 19133 Gerrit-PatchSet: 9 Gerrit-Owner: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 08 Nov 2022 16:08:03 +0000 Gerrit-HasComments: Yes
