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 15: Code-Review+1 (2 comments) One more nit, otherwise looks good to me. 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@907 PS13, Line 907: MetricUnit::Name(prototype_->unit()),value())); : DCHECK(false); : } : : S > The desired result should be fully empty, as leaving comments without actua Ah, now I see -- thank you for clarifying. http://gerrit.cloudera.org:8080/#/c/19133/15/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/19133/15/src/kudu/util/metrics.cc@902 PS15, Line 902: // This method implements the Prometheus format for string gauges, : // but it is not called anywhere, because Prometheus does not : // support string gauges. : writer->WriteEntry(Substitute("$0$1{unit_type=\"$2\"} $3\n", : prefix, prototype_->name(), : MetricUnit::Name(prototype_->unit()),value())); Instead of this piece of unused code that the compiler takes time to process but never uses, could we just leave here the following comment: A string gauge's value can be anything, but Prometheus does not support non-numeric values for gauges with exception of {+,-}Inf and NaN (see https://prometheus.io/docs/instrumenting/exposition_formats/). DCHECK() is added to make sure this method is not called from anywhere, but overriding it is necessary since Gauge::WriteValue() is a pure virtual one. An alternative could be defining a empty implementation for Gauge::WriteValue() virtual method and not adding this empty override here. -- 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: 15 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: Thu, 10 Nov 2022 02:21:59 +0000 Gerrit-HasComments: Yes
