Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/19133 )
Change subject: KUDU-3375 [metrics] Expose Prometheus Metrics in Kudu ...................................................................... Patch Set 13: (6 comments) http://gerrit.cloudera.org:8080/#/c/19133/13/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/19133/13/src/kudu/util/metrics.h@1030 PS13, Line 1030: > nit: the indent is off Done http://gerrit.cloudera.org:8080/#/c/19133/13/src/kudu/util/metrics.h@1415 PS13, Line 1415: > nit: the indent is off Done http://gerrit.cloudera.org:8080/#/c/19133/13/src/kudu/util/metrics.h@1492 PS13, Line 1492: > nit: the indent is off Done 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: if (s.IsNotFound()) { : // Status::NotFound is returned when this entity has been filtered, treat it : // as OK, and skip printing it. : return Status::OK(); : } > I still don't see the code that would handle other than Status::NotFound() // Get a snapshot of the entity's metrics as well as the entity's attributes, // maybe filtered by 'filters', see MetricFilters structure for details. // Return Status::NotFound when it has been filtered, or Status::OK when succeed. As you've mentioned previously, we can completely remove storing the status as well. Instead of making a new method to get metrics and attributes I've went along with already existing GetMetricsAndAttrs(). From the function comments and itself, returned Status is based on the passed filters. The filters that are passed to it are only for setting the level to Debug, which shouldl still return Status:OK() at any given time when the function is called. The code will fail if the GetMetricsAndAttributes() fails to work properly though, meaning if it doesn't assign the metrics and attributes http://gerrit.cloudera.org:8080/#/c/19133/13/src/kudu/util/metrics.cc@902 PS13, Line 902: writer->WriteEntry(Substitute("$0$1{unit_type=\"$2\"} $3\n", : prefix, prototype_->name(), : MetricUnit::Name(prototype_->unit()), value())); : } > If, according to the comment in StringGauge::WriteAsPrometheus(), string ga Removed the contents of the function to do nothing. Comment on line 911 explains it further. http://gerrit.cloudera.org:8080/#/c/19133/13/src/kudu/util/metrics.cc@907 PS13, Line 907: Status StringGauge::WriteAsPrometheus(PrometheusWriter* /*writer*/, : const std::string& /*prefix*/) const { : // Prometheus doesn't support string gauges : return Status::OK(); : } > There was a thread about this in PS6, and I missed following-up. As of PS1 Yes, I've removed the method. I've mistaken Gauge::WriteAsPrometheus() as a pure virtual function as well, that's why there is an empty implementation for it in StringGauge as well. I've removed the contents of the StringGauge::WriteValue() which will be used like you've mentioned when Gauge::WriteAsPrometheus() is called. -- 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: 13 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 19:59:05 +0000 Gerrit-HasComments: Yes
