Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/19133 )
Change subject: [metrics] Expose Prometheus Metrics in Kudu ...................................................................... Patch Set 7: (29 comments) http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@16 PS6, Line 16: according > according Done http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@17 PS6, Line 17: t > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@25 PS6, Line 25: kudu-master", > nit: kudu-master Done http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@32 PS6, Line 32: K > nit: extra space Done 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. > Is it possible to operate with such metrics as time-based ones then in Grap Yes, it is possible to use these units with the mentioned tools 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 = > If using variables instead of direct constants for arguments, could these b Converted to constant bool http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/CMakeLists.txt@224 PS6, Line 224: prometheus_write > nit: prometheus_writer.cc Done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.h@587 PS6, Line 587: > Why not to pass the prefix as a constant string? Done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.h@766 PS6, Line 766: virtual Status WriteAsPrometheus(PrometheusWriter* writer, const std::string& prefix) const = 0; : : > nit: remove them if not used. Done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.h@1275 PS6, Line 1275: writer->Value(value()); : } : > nit: how about using Substitute to build the string? 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: const auto& val > nit: could use structed binding (yes, it's C++17): https://en.cppreference. Couldn't figure out how to apply it to this situation exactly, could you please clarify it more in this case? 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 > What if this returns non-OK status? // Status::NotFound is returned when this entity has been filtered, treat it // as OK, and skip printing it. As for Prometheus we do not expose the possibility to filter the entities and this function will return all of the metrics if no filters are specified, thus eliminating the need to re-check the status to see if entry is filtered or not. As the MetricFilters are always empty, there no possibility of receiving any other status apart from OK from my understanding http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@415 PS6, Line 415: // Only emit server level metrics > nit for here and below: add space between next argument and the comma. Done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@416 PS6, Line 416: rcmp(prototype_->name(), "server") == > Here and below: why to convert into C string for comparison? Changed to == as id_ is a std::string not a const char* http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@418 PS6, Line 418: // attach kudu_master_ as prefix to metrics : WriteMetricsToPrometheus( > stringstream is a heavy and expensive object; use std::string or just const done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@431 PS6, Line 431: > What if prototype_->name() isn't "server" -- why to return Status::OK() in Extended the response, to return a NotFound status when the metric entity is not relevant to Prometheus. http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@590 PS6, Line 590: > nit: the indent is messed up Done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@733 PS6, Line 733: output = strings: > Why not just std::ostringstream? Converted to string http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@735 PS6, Line 735: prefix, name(),MetricType::Name(type())); : > There are many metrics with the same prototype, each of them will print HEL Yes, if we don't print Help or Type lines, the Description and Type do not get assigned in Prometheus http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@907 PS6, Line 907: const std::string& /*prefix*/) const { : // Prometheus doesn't support string gauges : return Status::OK(); : } > Using strings::Substitute() here instead would be more efficient, I guess. Done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@917 PS6, Line 917: ::lock_guard<simple_spinlock> l(lock_); > Then why to have this method at all? The abstract base class Gauge implements this function uses the WriteValue()(pure virtual) function to write this entry into Prometheus output. If not overridden this will lead to StringGauge to be written to Prometheus output as well. 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); : } > What sort of Prometheus metric does this map into? Is gauge allowed to hav Yes, we can represent gauge in multiple lines. All lines for a given metric must be provided as one single group, with the optional HELP and TYPE lines first (in no particular order). Groups are separated whenever there is a HELP, TYPE or a new line between the metric E.g #HELP metric_name description #TYPE metric_name gauge kudu_metric 0 kudu_metric_count 0 kudu_metric_sum 0 // new line #New line like above or new HELP/TYPE line results in a new metric #HELP new_metric Description #TYPE new_metric counter new_metric 0 http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@1109 PS6, Line 1109: gs::Substitu > why not just ostringstream? Done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@1111 PS6, Line 1111: MetricUnit::Name(prototype_->unit()), : snapshot.percent > nit for here and below: the indent is messed up Done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@1130 PS6, Line 1130: prefix, prototype_->name(), "_bucket", : MetricUnit::Name(prototype_->unit()), : snapsho > What if histogram_->TotalCount() value changes between this call and the ca Better solution would be to use a snapshot, and emit metrics based on that to keep consistency Another way to handle this, would be storing the TotalCount in a variable before this call and outputting it in both function calls. http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h File src/kudu/util/prometheuswriter.h: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h@17 PS6, Line 17: : > nit: use 'pragma' once instead Done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h@24 PS6, Line 24: : > nit: move this to be after the 'public' section Done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h@30 PS6, Line 30: > Pass constant input parameters by constant reference: https://google.github Done http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.cc File src/kudu/util/prometheuswriter.cc: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.cc@22 PS6, Line 22: : : > If the output data is converted into std::string anyways before writing int 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: 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: Tue, 18 Oct 2022 17:56:37 +0000 Gerrit-HasComments: Yes
