You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Khazar Mammadli (Code Review)" <ge...@cloudera.org> on 2022/10/12 18:49:26 UTC

[kudu-CR] [WIP] Expose Prometheus Metrics in Kudu

Khazar Mammadli has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19133


Change subject: [WIP] Expose Prometheus Metrics in Kudu
......................................................................

[WIP] Expose Prometheus Metrics in Kudu

This patch introduces an endpoint which emits server level metrics in
Prometheus Text Explosion format for Kudu.

More information on the Prometheus format can be found at:
https://prometheus.io/docs/instrumenting/exposition_formats

Tested by feeding all of the generated metrics to a prometheus server
running on localhost.

Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/default_path_handlers.h
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
A src/kudu/util/prometheuswriter.cc
A src/kudu/util/prometheuswriter.h
8 files changed, 221 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/19133/1
-- 
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: newchange
Gerrit-Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
Gerrit-Change-Number: 19133
Gerrit-PatchSet: 1
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>

[kudu-CR] [metrics] Expose Prometheus Metrics in Kudu

Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.

Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/19133 )

Change subject: [metrics] Expose Prometheus Metrics in Kudu
......................................................................


Patch Set 7:

(29 comments)

http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@16
PS6, Line 16: according
> according
Done


http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@17
PS6, Line 17: t
> nit: extra space
Done


http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@25
PS6, Line 25: kudu-master",
> nit: kudu-master
Done


http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@32
PS6, Line 32: K
> nit: extra space
Done


http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@34
PS6, Line 34: This also makes it possible to keep
            : time based units more accurate as Milliseconds, Nanoseconds and etc.
            : do not need to be converted to seconds which is the base time unit in Prometheus.
> Is it possible to operate with such metrics as time-based ones then in Grap
Yes, it is possible to use these units with the mentioned tools


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/server/default_path_handlers.cc
File src/kudu/server/default_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/server/default_path_handlers.cc@544
PS6, Line 544:   const bool not_styled = false;
             :   const bool is_on_nav_bar =
> If using variables instead of direct constants for arguments, could these b
Converted to constant bool


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/CMakeLists.txt@224
PS6, Line 224: prometheus_write
> nit: prometheus_writer.cc
Done


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

http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.h@587
PS6, Line 587: 
> Why not to pass the prefix as a constant string?
Done


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.h@766
PS6, Line 766:   virtual Status WriteAsPrometheus(PrometheusWriter* writer, const std::string& prefix) const = 0;
             : 
             : 
> nit: remove them if not used.
Done


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.h@1275
PS6, Line 1275: writer->Value(value());
              :   }
              : 
> nit: how about using Substitute to build the string?
Done


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@75
PS6, Line 75: const auto& val
> nit: could use structed binding (yes, it's C++17): https://en.cppreference.
Couldn't figure out how to apply it to this situation exactly, could you please clarify it more in this case?


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@413
PS6, Line 413:   // Thus, eliminating the need to check status message to 
> What if this returns non-OK status?
// Status::NotFound is returned when this entity has been filtered, treat it
// as OK, and skip printing it.

As for Prometheus we do not expose the possibility to filter the entities and this function will return all of the metrics if no filters are specified, thus eliminating the need to re-check the status to see if entry is filtered or not. 

As the MetricFilters are always empty, there no possibility of receiving any other status apart from OK from my understanding


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@415
PS6, Line 415:   // Only emit server level metrics
> nit for here and below: add space between next argument and the comma.
Done


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@416
PS6, Line 416: rcmp(prototype_->name(), "server") == 
> Here and below: why to convert into C string for comparison?
Changed to == as id_ is a std::string not a const char*


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@418
PS6, Line 418:      // attach kudu_master_ as prefix to metrics
             :       WriteMetricsToPrometheus(
> stringstream is a heavy and expensive object; use std::string or just const
done


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@431
PS6, Line 431: 
> What if prototype_->name() isn't "server" -- why to return Status::OK() in 
Extended the response, to return a NotFound status when the metric entity is not relevant to Prometheus.


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@590
PS6, Line 590: 
> nit: the indent is messed up
Done


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@733
PS6, Line 733:  output = strings:
> Why not just std::ostringstream?
Converted to string


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@735
PS6, Line 735:                              prefix, name(),MetricType::Name(type()));
             : 
> There are many metrics with the same prototype, each of them will print HEL
Yes, if we don't print Help or Type lines, the Description and Type do not get assigned in Prometheus


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@907
PS6, Line 907:                                       const std::string& /*prefix*/) const {
             :   // Prometheus doesn't support string gauges
             :   return Status::OK();
             : }
> Using strings::Substitute() here instead would be more efficient, I guess.
Done


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@917
PS6, Line 917: ::lock_guard<simple_spinlock> l(lock_);
> Then why to have this method at all?
The abstract base class Gauge implements this function uses the WriteValue()(pure virtual) function to write this entry into Prometheus output. If not overridden this will lead to StringGauge to be written to Prometheus output as well.


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@983
PS6, Line 983:   strings::SubstituteAndAppend(&output, "$0$1$2{unit_type=\"$3\"} $4\n",
             :                                prefix, prototype_->name(), "_sum",
             :                                MetricUnit::Name(prototype_->unit()), total_sum());
             : 
             :   writer->WriteEntry(output);
             : }
             : 
             : //
             : // Counter
             : //
             : // This implementation is optimized by using a striped counter. See LongAdder for details.
             : 
             : scoped_refptr<Counter> CounterPrototype::Instantiate(const scoped_refptr<MetricEntity>& entity) {
             :   return entity->FindOrCreateCounter(this);
             : }
> What sort of Prometheus metric does this map into?  Is gauge allowed to hav
Yes, we can represent gauge in multiple lines. All lines for a given metric must be provided as one single group, with the optional HELP and TYPE lines first (in no particular order). Groups are separated whenever there is a HELP, TYPE or a new line between the metric

E.g

#HELP metric_name description
#TYPE metric_name gauge
kudu_metric 0
kudu_metric_count 0
kudu_metric_sum 0
// new line
#New line like above or new HELP/TYPE line results in a new metric 
#HELP new_metric Description
#TYPE new_metric counter
new_metric 0


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@1109
PS6, Line 1109: gs::Substitu
> why not just ostringstream?
Done


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@1111
PS6, Line 1111:                                MetricUnit::Name(prototype_->unit()),
              :                                snapshot.percent
> nit for here and below: the indent is messed up
Done


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@1130
PS6, Line 1130:                                prefix, prototype_->name(), "_bucket",
              :                                MetricUnit::Name(prototype_->unit()),
              :                                snapsho
> What if histogram_->TotalCount() value changes between this call and the ca
Better solution would be to use a snapshot, and emit metrics based on that to keep consistency

Another way to handle this, would be storing the TotalCount in a variable before this call and outputting it in both function calls.


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h
File src/kudu/util/prometheuswriter.h:

http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h@17
PS6, Line 17: 
            : 
> nit: use 'pragma' once instead
Done


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h@24
PS6, Line 24: 
            : 
> nit: move this to be after the 'public' section
Done


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h@30
PS6, Line 30: 
> Pass constant input parameters by constant reference: https://google.github
Done


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.cc
File src/kudu/util/prometheuswriter.cc:

http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.cc@22
PS6, Line 22: 
            : 
            : 
> If the output data is converted into std::string anyways before writing int
Done



-- 
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: 7
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 18 Oct 2022 17:56:37 +0000
Gerrit-HasComments: Yes

[kudu-CR] [metrics] Expose Prometheus Metrics in Kudu

Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Yingchun Lai, Attila Bukor, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19133

to look at the new patch set (#12).

Change subject: [metrics] Expose Prometheus Metrics in Kudu
......................................................................

[metrics] Expose Prometheus Metrics in Kudu

This patch introduces an endpoint which emits server level metrics in
Prometheus Text Explosion format for Kudu. The endpoint can be found at
/metrics_prometheus path of the webserver.

More information on the Prometheus format can be found at:
https://prometheus.io/docs/instrumenting/exposition_formats

Prefix kudu_ has been added to all of the metric names according to
Prometheus naming convention to specify the domain and differentiate
from any other metrics belonging to different services. Following the
kudu_ prefix either master_ or tserver_ prefix is attached based on the
server type that is exposing the metrics. In order to differentiate
between different master and tablet servers running on the same host
Prometheus automatically attaches an instance label which holds the
"hostname:portname".

eg: kudu_master_metric{instance="localhost:8765", job="kudu-master", unit_type="unit"}

Not all of the Kudu metric names follow the Prometheus metric naming
recommendations which state metric units should be attached at the end
of the metric name. In Kudu some metrics have these units at the end of
the metric name, some in the middle of the metric and for some metrics
it is unclear what the metric unit is just by looking at the metric
name. Due to this and also because Kudu supports more Metric Units
than base Prometheus Metric units, metric units are represented as
Prometheus labels for each metric. This also makes it possible to keep
time based units more accurate as Milliseconds, Nanoseconds and etc.
do not need to be converted to seconds which is the base time unit in Prometheus.

More information on Prometheus Metric units and naming conventions can be found at:
https://prometheus.io/docs/practices/naming/#metric-names

Tested by feeding all of the generated metrics to a prometheus server
running on localhost. Added unit tests to each metric type.

Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/default_path_handlers.h
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
A src/kudu/util/prometheus_writer.cc
A src/kudu/util/prometheus_writer.h
9 files changed, 440 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/19133/12
-- 
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: newpatchset
Gerrit-Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
Gerrit-Change-Number: 19133
Gerrit-PatchSet: 12
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] [metrics] Expose Prometheus Metrics in Kudu

Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.

Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/19133 )

Change subject: [metrics] Expose Prometheus Metrics in Kudu
......................................................................


Patch Set 9:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/server/default_path_handlers.cc
File src/kudu/server/default_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/server/default_path_handlers.cc@544
PS6, Line 544:   constexpr bool not_styled = false;
             :   constexpr bool is_on_nav_b
> nit: constexpr might be even better since it opens more opportunities for c
Done


http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/server/server_base.cc@972
PS7, Line 972: ,
> nit: add a space after comma
done


http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics-test.cc
File src/kudu/util/metrics-test.cc:

http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics-test.cc@308
PS7, Line 308: ASSERT_EQ(expected_output, output.str());
> nit for here and below: the reference/expected value should come first, oth
done


http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics-test.cc@403
PS7, Line 403: const string expected_outpu
> nit for here and other places: consider adding 'const' to be more explicit 
done


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

http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.h@586
PS7, Line 586: & 
> style nit: stick the ampersand to the type
done


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@75
PS6, Line 75:                
> It's something like
Done, thanks for clarifying


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@413
PS6, Line 413:   const string master_server = "kudu.master";
> Alright, but then why to store the status of the GetMetricsAndAttrs() call 
Re added the status check for NotFound() to skip over filtered entities. If there are any extra cases that also should be considered that come to your mind, please do share


http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@983
PS6, Line 983:                        prefix, prototype_->name(), "_sum",
             :                        MetricUnit::Name(prototype_->unit()), total_sum());
             : 
             :   writer->WriteEntry(output);
             : }
             : 
             : //
             : // Counter
             : //
             : // This implementation is optimized by using a striped counter. See LongAdder for details.
             : 
             : scoped_refptr<Counter> CounterPrototype::Instantiate(const scoped_refptr<MetricEntity>& entity) {
             :   return entity->FindOrCreateCounter(this);
             : }
             : 
> Cool, thanks for clarifying on this.
Can't find an explicit location on where it is mentioned in the documentation but I have seen use cases, and they can be either ignored or utilized by the users. This explanation itself is provided in the commit with the text explosion format.


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

http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@417
PS7, Line 417: tMetricsAndAt
> nit: consider defining a constant for this and for "kudu.tabletserver" as w
done


http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@731
PS7, Line 731:     writer->String(description());
             : 
             :     writer->String("level");
             :     writer->String(MetricLevelName(level()));
             :   }
             : }
             : 
> nit for here and below: could get rid of temporary variable and the assignm
Done


http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@900
PS7, Line 900: 
> nit for here and elsewher: drop the 'strings::' namespace prefix since this
Done


http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@974
PS7, Line 974: void MeanGauge::WriteValue(PrometheusWriter* writer, const std::string& prefix) const {
             :   auto output = Substitute("$0$1{unit_type=\"$2\"} $3\n", prefix, prototype_->name(),
             :           
> nit: could join these into
Done


http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@1100
PS7, Line 1100: 
              :   output = Substitute("$0$1$2{unit_type=\"$3\", le=\"0.75\"} $4\n",
              :                        prefix, prototype_->name(), "_buck
> nit: please add a comment to explain that the snapshot is taken to preserve
done


http://gerrit.cloudera.org:8080/#/c/19133/7/src/kudu/util/metrics.cc@1109
PS7, Line 1109:                      snapsho
> nit: remove the 'strings::' namespace prefix and add 'using strings::Substi
Done



-- 
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: 9
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Nov 2022 16:08:03 +0000
Gerrit-HasComments: Yes

[kudu-CR] [metrics] Expose Prometheus Metrics in Kudu

Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Yingchun Lai, Attila Bukor, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19133

to look at the new patch set (#8).

Change subject: [metrics] Expose Prometheus Metrics in Kudu
......................................................................

[metrics] Expose Prometheus Metrics in Kudu

This patch introduces an endpoint which emits server level metrics in
Prometheus Text Explosion format for Kudu. The endpoint can be found at
/metrics_prometheus path of the webserver.

More information on the Prometheus format can be found at:
https://prometheus.io/docs/instrumenting/exposition_formats

Prefix kudu_ has been added to all of the metric names according to
Prometheus naming convention to specify the domain and differentiate
from any other metrics belonging to different services. Following the
kudu_ prefix either master_ or tserver_ prefix is attached based on the
server type that is exposing the metrics. In order to differentiate
between different master and tablet servers running on the same host
Prometheus automatically attaches an instance label which holds the
"hostname:portname".

eg: kudu_master_metric{instance="localhost:8765", job="kudu-master", unit_type="unit"}

Not all of the Kudu metric names follow the Prometheus metric naming
recommendations which state metric units should be attached at the end
of the metric name. In Kudu some metrics have these units at the end of
the metric name, some in the middle of the metric and for some metrics
it is unclear what the metric unit is just by looking at the metric
name. Due to this and also because Kudu supports more Metric Units
than base Prometheus Metric units, metric units are represented as
Prometheus labels for each metric. This also makes it possible to keep
time based units more accurate as Milliseconds, Nanoseconds and etc.
do not need to be converted to seconds which is the base time unit in Prometheus.

More information on Prometheus Metric units and naming conventions can be found at:
https://prometheus.io/docs/practices/naming/#metric-names

Tested by feeding all of the generated metrics to a prometheus server
running on localhost. Added unit tests to each metric type.

Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/default_path_handlers.h
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
A src/kudu/util/prometheus_writer.cc
A src/kudu/util/prometheus_writer.h
9 files changed, 439 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/19133/8
-- 
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: newpatchset
Gerrit-Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
Gerrit-Change-Number: 19133
Gerrit-PatchSet: 8
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] [metrics] Expose Prometheus Metrics in Kudu

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19133 )

Change subject: [metrics] Expose Prometheus Metrics in Kudu
......................................................................


Patch Set 12: Code-Review+1

(1 comment)

Overall looks good to me, please address Yingchun's comments and one more nit.

http://gerrit.cloudera.org:8080/#/c/19133/11//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19133/11//COMMIT_MSG@36
PS11, Line 36: do not need to be converted to seconds which is the base time unit in Prometheus.
             : 
             : More information on Prometheus Metric units and naming conventions can be found at:
nit: strive to keep lines' length under 72 symbols, the guide is at https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines  (it can be found at the https://kudu.apache.org/docs/contributing.html#_submitting_patches page).



-- 
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: 12
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Nov 2022 16:00:51 +0000
Gerrit-HasComments: Yes

[kudu-CR] [metrics] Expose Prometheus Metrics in Kudu

Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Zoltan Chovan, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19133

to look at the new patch set (#5).

Change subject: [metrics] Expose Prometheus Metrics in Kudu
......................................................................

[metrics] Expose Prometheus Metrics in Kudu

This patch introduces an endpoint which emits server level metrics in
Prometheus Text Explosion format for Kudu. The endpoint can be found at
/metrics_prometheus path of the webserver.

More information on the Prometheus format can be found at:
https://prometheus.io/docs/instrumenting/exposition_formats

Prefix kudu_ has been added to all of the metric names accroding to
Prometheus naming convention  to specify the domain and differentiate
from any other metrics belonging to different services. Following the
kudu_ prefix either master_ or tserver_ prefix is attached based on the
server type that is exposing the metrics. In order to differentiate
between different master and tablet servers running on the same host
Prometheus automatically attaches an instance label which holds the
"hostname:portname".

eg: kudu_master_metric{instance="localhost:8765", job="‘kudu-master’", unit_type="unit"}

Not all of the Kudu metric names follow the Prometheus metric naming
recommendations which state metric units should be attached at the end
of the metric name. In Kudu some metrics have these units at the end of
the metric name, some in the middle of the metric and for some metrics
it is unclear what the metric unit is just by looking at the metric
name. Due to this and also because  Kudu supports more Metric Units
than base Prometheus Metric units, metric units are represented as
Prometheus labels for each metric. This also makes it possible to keep
time based units more accurate as Milliseconds, Nanoseconds and etc.
do not need to be converted to seconds which is the base time unit in Prometheus.

More information on Prometheus Metric units and naming conventions can be found at:
https://prometheus.io/docs/practices/naming/#metric-names

Tested by feeding all of the generated metrics to a prometheus server
running on localhost. Added unit tests to each metric type.

Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/default_path_handlers.h
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
A src/kudu/util/prometheuswriter.cc
A src/kudu/util/prometheuswriter.h
9 files changed, 409 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/19133/5
-- 
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: newpatchset
Gerrit-Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
Gerrit-Change-Number: 19133
Gerrit-PatchSet: 5
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3375 Expose Prometheus Metrics in Kudu

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

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 16: Code-Review+2


-- 
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: 16
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 03:42:30 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3375 Expose Prometheus Metrics in Kudu

Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Yingchun Lai, Attila Bukor, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19133

to look at the new patch set (#14).

Change subject: KUDU-3375 Expose Prometheus Metrics in Kudu
......................................................................

KUDU-3375 Expose Prometheus Metrics in Kudu

This patch introduces an endpoint which emits server level metrics in
Prometheus Text Explosion format for Kudu. The endpoint can be found at
/metrics_prometheus path of the webserver.

More information on the Prometheus format can be found at:
https://prometheus.io/docs/instrumenting/exposition_formats

Prefix kudu_ has been added to all of the metric names according to
Prometheus naming convention to specify the domain and differentiate
from any other metrics belonging to different services. Following the
kudu_ prefix either master_ or tserver_ prefix is attached based on the
server type that is exposing the metrics. In order to differentiate
between different master and tablet servers running on the same host
Prometheus automatically attaches an instance label which holds the
"hostname:portname".

eg: kudu_master_metric{instance="localhost:8765", job="kudu-master", unit_type="unit"}

Not all of the Kudu metric names follow the Prometheus metric naming
recommendations which state metric units should be attached at the end
of the metric name. In Kudu some metrics have these units at the end of
the metric name, some in the middle of the metric and for some metrics
it is unclear what the metric unit is just by looking at the metric
name. Due to this and also because Kudu supports more Metric Units
than base Prometheus Metric units, metric units are represented as
Prometheus labels for each metric. This also makes it possible to keep
time based units more accurate as Milliseconds, Nanoseconds and etc.
do not need to be converted to seconds which is the base time unit in
Prometheus.

More information on Prometheus Metric units and naming conventions
can be found at:
https://prometheus.io/docs/practices/naming/#metric-names

Tested by feeding all of the generated metrics to a prometheus server
running on localhost. Added unit tests to each metric type.

Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/default_path_handlers.h
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
A src/kudu/util/prometheus_writer.cc
A src/kudu/util/prometheus_writer.h
9 files changed, 426 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/19133/14
-- 
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: newpatchset
Gerrit-Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
Gerrit-Change-Number: 19133
Gerrit-PatchSet: 14
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] [metrics] Expose Prometheus Metrics in Kudu

Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Yingchun Lai, Attila Bukor, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19133

to look at the new patch set (#7).

Change subject: [metrics] Expose Prometheus Metrics in Kudu
......................................................................

[metrics] Expose Prometheus Metrics in Kudu

This patch introduces an endpoint which emits server level metrics in
Prometheus Text Explosion format for Kudu. The endpoint can be found at
/metrics_prometheus path of the webserver.

More information on the Prometheus format can be found at:
https://prometheus.io/docs/instrumenting/exposition_formats

Prefix kudu_ has been added to all of the metric names according to
Prometheus naming convention to specify the domain and differentiate
from any other metrics belonging to different services. Following the
kudu_ prefix either master_ or tserver_ prefix is attached based on the
server type that is exposing the metrics. In order to differentiate
between different master and tablet servers running on the same host
Prometheus automatically attaches an instance label which holds the
"hostname:portname".

eg: kudu_master_metric{instance="localhost:8765", job="kudu-master", unit_type="unit"}

Not all of the Kudu metric names follow the Prometheus metric naming
recommendations which state metric units should be attached at the end
of the metric name. In Kudu some metrics have these units at the end of
the metric name, some in the middle of the metric and for some metrics
it is unclear what the metric unit is just by looking at the metric
name. Due to this and also because Kudu supports more Metric Units
than base Prometheus Metric units, metric units are represented as
Prometheus labels for each metric. This also makes it possible to keep
time based units more accurate as Milliseconds, Nanoseconds and etc.
do not need to be converted to seconds which is the base time unit in Prometheus.

More information on Prometheus Metric units and naming conventions can be found at:
https://prometheus.io/docs/practices/naming/#metric-names

Tested by feeding all of the generated metrics to a prometheus server
running on localhost. Added unit tests to each metric type.

Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/default_path_handlers.h
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
A src/kudu/util/prometheus_writer.cc
A src/kudu/util/prometheus_writer.h
9 files changed, 442 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/19133/7
-- 
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: newpatchset
Gerrit-Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
Gerrit-Change-Number: 19133
Gerrit-PatchSet: 7
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3375 [metrics] Expose Prometheus Metrics in Kudu

Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.

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 <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Nov 2022 19:59:05 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3375 Expose Prometheus Metrics in Kudu

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

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 <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 02:21:59 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3375 Expose Prometheus Metrics in Kudu

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19133 )

Change subject: KUDU-3375 Expose Prometheus Metrics in Kudu
......................................................................

KUDU-3375 Expose Prometheus Metrics in Kudu

This patch introduces an endpoint which emits server level metrics in
Prometheus Text Explosion format for Kudu. The endpoint can be found at
/metrics_prometheus path of the webserver.

More information on the Prometheus format can be found at:
https://prometheus.io/docs/instrumenting/exposition_formats

Prefix kudu_ has been added to all of the metric names according to
Prometheus naming convention to specify the domain and differentiate
from any other metrics belonging to different services. Following the
kudu_ prefix either master_ or tserver_ prefix is attached based on the
server type that is exposing the metrics. In order to differentiate
between different master and tablet servers running on the same host
Prometheus automatically attaches an instance label which holds the
"hostname:portname".

eg: kudu_master_metric{instance="localhost:8765", job="kudu-master", unit_type="unit"}

Not all of the Kudu metric names follow the Prometheus metric naming
recommendations which state metric units should be attached at the end
of the metric name. In Kudu some metrics have these units at the end of
the metric name, some in the middle of the metric and for some metrics
it is unclear what the metric unit is just by looking at the metric
name. Due to this and also because Kudu supports more Metric Units
than base Prometheus Metric units, metric units are represented as
Prometheus labels for each metric. This also makes it possible to keep
time based units more accurate as Milliseconds, Nanoseconds and etc.
do not need to be converted to seconds which is the base time unit in
Prometheus.

More information on Prometheus Metric units and naming conventions
can be found at:
https://prometheus.io/docs/practices/naming/#metric-names

Tested by feeding all of the generated metrics to a prometheus server
running on localhost. Added unit tests to each metric type.

Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
Reviewed-on: http://gerrit.cloudera.org:8080/19133
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/default_path_handlers.h
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
A src/kudu/util/prometheus_writer.cc
A src/kudu/util/prometheus_writer.h
9 files changed, 445 insertions(+), 14 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, approved

-- 
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: merged
Gerrit-Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739
Gerrit-Change-Number: 19133
Gerrit-PatchSet: 17
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] [metrics] Expose Prometheus Metrics in Kudu

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19133 )

Change subject: [metrics] Expose Prometheus Metrics in Kudu
......................................................................


Patch Set 6:

(1 comment)

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@75
PS6, Line 75: const auto& val
> Couldn't figure out how to apply it to this situation exactly, could you pl
It's something like

for (const auto& [name, val] : metrics) {
   ...
}

where 'name' is for '.first', and 'val' is for '.second' of the elements being iterated, there isn't a need to introduce extra variables in the scope of the for() cycle.



-- 
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: 6
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Oct 2022 17:31:19 +0000
Gerrit-HasComments: Yes