Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19133 )
Change subject: KUDU-3375 Expose Prometheus Metrics in Kudu ...................................................................... Patch Set 14: (3 comments) http://gerrit.cloudera.org:8080/#/c/19133/13/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/19133/13/src/kudu/util/metrics.cc@418 PS13, Line 418: // Only emit server level metrics : if (strcmp(prototype_->name(), "server") == 0) { : if (id_ == master_server) { : // attach kudu_master_ as prefix to metrics : > // Get a snapshot of the entity's metrics as well as the entity's attribute Could you please just add proper status code handling here for GetMetricsAndAttrs()? That's what I think is missing here to make the code robust and future-proof regarding GetMetricsAndAttrs(). I guess that might look like the following: ... const auto s = GetMetricsAndAttrs(filters, &metrics, &attrs); if (s.IsNotFound()) { // Status::NotFound is returned when this entity has been filtered, treat it // as OK, and skip printing it. return Status::OK(); } RETURN_NOT_OK(s); ... The important piece is adding having RETURN_NOT_OK(s) to handle other than Status::NotFound() status. http://gerrit.cloudera.org:8080/#/c/19133/13/src/kudu/util/metrics.cc@907 PS13, Line 907: p->set_value(total_sum_, total_count_); : p->m_epoch_.store(m_epoch_); : p->invalid_for_merge_ = invalid_for_merge_; : p->retire_time_ = retire_time_; : > Yes, I've removed the method. I've mistaken Gauge::WriteAsPrometheus() as a OK, thanks for the update. Since I guess there is something I'm still missing here, I want to clarify on that. So, what do we want to see as a result when calling WriteAsPrometheus() for a string gauge: should we have a completely empty output or we still want the comment to be output as per MetricPrototype::WriteFields() implementation? If a completely empty output is desired, maybe your original approach of overriding StringGauge::WriteAsPrometheus() that does nothing but returns Status::OK() is the desired goal? And if it's indeed so, then we want that instead of relying on Gauge::WriteAsPrometheus(), and maybe we also want to add DCHECK(false) into the implementation of the StringGauge::WriteValue(PrometheusWriter*, const std::string&) method to make sure it's not called from anywhere? Thanks! http://gerrit.cloudera.org:8080/#/c/19133/14/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/19133/14/src/kudu/util/metrics.cc@896 PS14, Line 896: void StringGauge::WriteValue(PrometheusWriter* writer, const std::string& prefix) const { Please add a comment to explain why this method does nothing, otherwise it looks like the implementation was omitted by a mistake. -- 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: 14 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: Wed, 09 Nov 2022 22:21:34 +0000 Gerrit-HasComments: Yes
