Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19133 )
Change subject: [metrics] Expose Prometheus Metrics in Kudu ...................................................................... Patch Set 7: (14 comments) http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@34 PS6, Line 34: This also makes it possible to keep : time based units more accurate as Milliseconds, Nanoseconds and etc. : do not need to be converted to seconds which is the base time unit in Prometheus. > Yes, it is possible to use these units with the mentioned tools Cool -- glad to hear it's not an obstacle for the tools, so we don't need to expose everything in seconds. 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: const bool not_styled = false; : const bool is_on_nav_bar = > Converted to constant bool nit: constexpr might be even better since it opens more opportunities for compile-time optimization 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 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(output.str(), expected_output); nit for here and below: the reference/expected value should come first, otherwise it's harder to comprehend the output if this assertion ever triggers http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics-test.cc@403 PS7, Line 403: std::string expected_output nit for here and other places: consider adding 'const' to be more explicit on the immutable semantics of the references 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 Also, consider having parameter by default there for prefix to be empty, so it's not necessary to supply empty strings in case where prefix isn't relevant 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@413 PS6, Line 413: // Thus, eliminating the need to check status message to > // Status::NotFound is returned when this entity has been filtered, treat i Alright, but then why to store the status of the GetMetricsAndAttrs() call at all? If it returned NotFound(), shouldn't the code short-circuit right here and return from this method/function? Also, what if GetMetricsAndAttrs() changes to start returning something beside Status::OK() and Status::NotFound()? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@983 PS6, Line 983: strings::SubstituteAndAppend(&output, "$0$1$2{unit_type=\"$3\"} $4\n", : 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); : } > Yes, we can represent gauge in multiple lines. All lines for a given metric Cool, thanks for clarifying on this. Could you please also add a reference to the documentation where it's clear (or explicitly said so) that a gauge is allowed to have multiple values like in your example? 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: "kudu.master" nit: consider defining a constant for this and for "kudu.tabletserver" as well as you did for the prefixes http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@731 PS7, Line 731: std::string output = ""; : : output = strings::Substitute("# HELP $0$1 $2\n# TYPE $3$4 $5\n", : prefix, name(), description(), : prefix, name(),MetricType::Name(type())); : : writer->WriteEntry(output); nit for here and below: could get rid of temporary variable and the assignment operator writer->WriteEntry(Substitute(...)); http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@900 PS7, Line 900: strings:: nit for here and elsewher: drop the 'strings::' namespace prefix since this file already has 'using strings::Substitute' http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@974 PS7, Line 974: std::string output = ""; : : output = nit: could join these into auto string = Substitute(...); http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@1100 PS7, Line 1100: MetricJsonOptions opts; : HistogramSnapshotPB snapshot; : RETURN_NOT_OK(GetHistogramSnapshotPB(&snapshot, opts)); nit: please add a comment to explain that the snapshot is taken to preserve consistency across the metrics and to satisfy the Prometheus' requirement on the '_bucket' and '_count' values. http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@1109 PS7, Line 1109: strings::SubstituteAndAppend nit: remove the 'strings::' namespace prefix and add 'using strings::SubstituteAndAppend' in the beginning of this file -- 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: 7 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: Fri, 28 Oct 2022 18:30:33 +0000 Gerrit-HasComments: Yes
