Yan-Daojiang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/24790 )

Change subject: [metrics] Support quantile selection in /metrics_prometheus
......................................................................


Patch Set 2:

(4 comments)

All comments have been addressed.
PTAL when you have time.

http://gerrit.cloudera.org:8080/#/c/24790/1/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:

http://gerrit.cloudera.org:8080/#/c/24790/1/src/kudu/util/metrics.h@586
PS1, Line 586:   // 'quantile' label ('0', '0.75', '
> Since only preset quantile label are ever added into this container and the
Done


http://gerrit.cloudera.org:8080/#/c/24790/1/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:

http://gerrit.cloudera.org:8080/#/c/24790/1/src/kudu/util/metrics.cc@43
PS1, Line 43: using strings::Substitute;
            : using strings::SubstituteAndAppend;
            :
            : namespace {
            : // The quantile lines a histogram exports in Prometheus format, 
in output order.
            : // 'tag' is the value of the Prometheus 'quantile' label and the 
token accepted
            : // by --metrics_prometheus_default_quantiles and the 'quantiles' 
query parameter.
            : // The min ('0') and max ('1') export the exact recorded extrema; 
the others
            : // export the value at 'percentile' (as passed to 
HdrHistogram::ValueAtPercentile).
            : // This is the single canonical description of the preset 
quantiles.
            : struct HistogramQuantile {
            :
> Does it makes sense to make these contstexpr?
Done


http://gerrit.cloudera.org:8080/#/c/24790/1/src/kudu/util/metrics.cc@153
PS1, Line 153: TAG_FLAG(metrics_prometheus_default_quantiles, runtime);
             : TAG_FLAG(metrics_prometheus_default_quantiles, evolving);
             : DEFINE_validator(metrics_prometheus_default_quantiles,
             :                  [](const char* flag_name, const string& value) {
             :   // An empty value exports all qua
> nit: maybe, move this block to come before flag validators, and omit the 's
Done


http://gerrit.cloudera.org:8080/#/c/24790/1/src/kudu/util/metrics.cc@1413
PS1, Line 1413: }
              :
              : void Histogram::IncrementBy(int64_t value, int64_t amount) {
              :   UpdateModificationEpoch();
              :   histogram_->IncrementBy(value, amount);
              : }
              :
              : Status Histogram::WriteAsJson(JsonWriter* writer,
              :                               const MetricJsonOptions& opts) 
const {
              :
> Is there a chance to unify this and the newly introduced kHistogramQuantile
Done



--
To view, visit http://gerrit.cloudera.org:8080/24790
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I01b48aa30f26f2a7efcd842bf6512a023cbf90ff
Gerrit-Change-Number: 24790
Gerrit-PatchSet: 2
Gerrit-Owner: Yan-Daojiang <[email protected]>
Gerrit-Reviewer: Alexey Serbin <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <[email protected]>
Gerrit-Reviewer: Yan-Daojiang <[email protected]>
Gerrit-Comment-Date: Mon, 07 Sep 2026 06:22:49 +0000
Gerrit-HasComments: Yes

Reply via email to