Alexey Serbin 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: (7 comments) http://gerrit.cloudera.org:8080/#/c/19133/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19133/13//COMMIT_MSG@7 PS13, Line 7: KUDU-3375 [metrics] nit: just for future reference, please use either 'KUDU-3375' or '[metrics]', but not both (it also helps to keep the summary under 50 symbols in length) 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 http://gerrit.cloudera.org:8080/#/c/19133/13/src/kudu/util/metrics.h@1415 PS13, Line 1415: nit: the indent is off http://gerrit.cloudera.org:8080/#/c/19133/13/src/kudu/util/metrics.h@1492 PS13, Line 1492: nit: the indent is off 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@917 PS6, Line 917: d_refptr<Metric> MeanGauge::snapshot() c > The abstract base class Gauge implements this function uses the WriteValue( If not overridden, the implementation will just use WriteValue() that's been overridden by the custom implementation for StringGauge::WriteValue(), isn't it? With that, should we remove this method? 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() error status at this point. Current implementation of GetMetricsAndAttrs() is one thing, but the code should provide clear semantics of what happens in case of other error codes returned. Also, the implementation of GetMetricsAndAttrs() may change in the future and start returning other status codes. As of now, Status::OK() and other than Status::NotFound() are treated exactly the same in this code, I don't think it's OK. 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 PS13, I'm still confused with this. If not overridden, the implementation of Gauge::WriteAsPrometheus() will use WriteValue() that's been overridden by the custom implementation for StringGauge::WriteValue(), isn't it? With that, should we remove this method? -- 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 18:32:53 +0000 Gerrit-HasComments: Yes
