This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push: new 6b1c1eb0c KUDU-3549 fix WriteAsPrometheus() for non-arithmetic gauges 6b1c1eb0c is described below commit 6b1c1eb0c97a2349e0b3fa098bf40f8147b43a60 Author: Alexey Serbin <ale...@apache.org> AuthorDate: Fri Feb 2 00:08:39 2024 -0800 KUDU-3549 fix WriteAsPrometheus() for non-arithmetic gauges This patch fixes WriteAsPrometheus() implementation for string-based gauges. The original changelist that introduced Prometheus metrics had a proper implementation of WriteAsPrometheus() only for StringGauge, but any FunctionGauge for a non-arithmetic type (e.g., for std::string) would still output a string value in Prometheus text format, and that could not be consumed by Prometheus. The patch also contains a test scenario that would fail without the fix. This is a follow-up to 00efc6826ac9a1f5d10750296c7357790a041fec. Change-Id: Ib7128f52729c7f984004811153a7eecc8ffe751b Reviewed-on: http://gerrit.cloudera.org:8080/20990 Tested-by: Alexey Serbin <ale...@apache.org> Reviewed-by: Marton Greber <greber...@gmail.com> Reviewed-by: Attila Bukor <abu...@apache.org> --- src/kudu/util/metrics-test.cc | 28 ++++++++++++-- src/kudu/util/metrics.cc | 39 +++++++++----------- src/kudu/util/metrics.h | 86 ++++++++++++++++++++++++------------------- 3 files changed, 90 insertions(+), 63 deletions(-) diff --git a/src/kudu/util/metrics-test.cc b/src/kudu/util/metrics-test.cc index 2f3bc6a75..8d598ebc5 100644 --- a/src/kudu/util/metrics-test.cc +++ b/src/kudu/util/metrics-test.cc @@ -202,23 +202,22 @@ TEST_F(MetricsTest, SimpleStringGaugeForMergeTest) { state_for_merge->unique_values()); } -TEST_F(MetricsTest, StringGaugePrometheusTest) { +TEST_F(MetricsTest, StringGaugeForPrometheus) { scoped_refptr<StringGauge> state = new StringGauge(&METRIC_test_string_gauge, "Healthy"); ostringstream output; PrometheusWriter writer(&output); ASSERT_OK(state->WriteAsPrometheus(&writer, {})); - // String Gauges are not representable in Prometheus, empty output is expected + // String-based gauges are not consumable by Prometheus. ASSERT_EQ("", output.str()); - // Make sure proper methods are called when relying on method overrides. + // Test that proper methods are called when relying on virtual overrides. { const Metric* g = state.get(); ostringstream output; PrometheusWriter writer(&output); ASSERT_OK(g->WriteAsPrometheus(&writer, {})); - // String Gauges are not representable in Prometheus, empty output is expected ASSERT_EQ("", output.str()); } { @@ -230,6 +229,27 @@ TEST_F(MetricsTest, StringGaugePrometheusTest) { } } +METRIC_DEFINE_gauge_string(test_entity, + string_function_gauge, + "String Function Gauge", + MetricUnit::kState, + "Description of string function gauge", + kudu::MetricLevel::kInfo); + +TEST_F(MetricsTest, StringFunctionGaugeForPrometheus) { + scoped_refptr<FunctionGauge<string>> gauge = + METRIC_string_function_gauge.InstantiateFunctionGauge( + entity_, []() { return "string function gauge"; }); + FunctionGaugeDetacher detacher; + gauge->AutoDetachToLastValue(&detacher); + + ostringstream output; + PrometheusWriter writer(&output); + ASSERT_OK(gauge->WriteAsPrometheus(&writer, {})); + // String-based gauges are not consumable by Prometheus. + ASSERT_EQ("", output.str()); +} + METRIC_DEFINE_gauge_double(test_entity, test_mean_gauge, "Test mean Gauge", MetricUnit::kUnits, "Description of mean Gauge", kudu::MetricLevel::kInfo); diff --git a/src/kudu/util/metrics.cc b/src/kudu/util/metrics.cc index 22cf785f2..dfcea6db1 100644 --- a/src/kudu/util/metrics.cc +++ b/src/kudu/util/metrics.cc @@ -17,6 +17,7 @@ #include "kudu/util/metrics.h" #include <iostream> +#include <tuple> #include <utility> #include <gflags/gflags.h> @@ -69,13 +70,13 @@ void WriteMetricsToJson(JsonWriter* writer, writer->EndArray(); } -template<typename Collection> -void WriteMetricsToPrometheus(PrometheusWriter* writer, - const Collection& metrics, - const string& prefix) { +void WriteMetricsPrometheus(PrometheusWriter* writer, + const MetricEntity::MetricMap& metrics, + const string& prefix) { for (const auto& [name, val] : metrics) { WARN_NOT_OK(val->WriteAsPrometheus(writer, prefix), - Substitute("Failed to write $0 as Prometheus", name)); + Substitute("unable to write '$0' ($1) in Prometheus format", + name, val->prototype()->description())); } } @@ -424,12 +425,12 @@ Status MetricEntity::WriteAsPrometheus(PrometheusWriter* writer) const { if (strcmp(prototype_->name(), "server") == 0) { if (id_ == master_server) { // attach kudu_master_ as prefix to metrics - WriteMetricsToPrometheus(writer, metrics, master_prefix); + WriteMetricsPrometheus(writer, metrics, master_prefix); return Status::OK(); } if (id_ == tablet_server) { // attach kudu_tserver_ as prefix to metrics - WriteMetricsToPrometheus(writer, metrics, tserver_prefix); + WriteMetricsPrometheus(writer, metrics, tserver_prefix); return Status::OK(); } } @@ -735,7 +736,8 @@ void MetricPrototype::WriteFields(JsonWriter* writer, } } -void MetricPrototype::WriteFields(PrometheusWriter* writer, const string &prefix) const { +void MetricPrototype::WriteHelpAndType(PrometheusWriter* writer, + const string& prefix) const { writer->WriteEntry(Substitute("# HELP $0$1 $2\n# TYPE $3$4 $5\n", prefix, name(), description(), prefix, name(), MetricType::Name(type()))); @@ -819,8 +821,7 @@ Status Gauge::WriteAsJson(JsonWriter* writer, } Status Gauge::WriteAsPrometheus(PrometheusWriter* writer, const string& prefix) const { - prototype_->WriteFields(writer, prefix); - + prototype_->WriteHelpAndType(writer, prefix); WriteValue(writer, prefix); return Status::OK(); @@ -1031,11 +1032,9 @@ Status Counter::WriteAsJson(JsonWriter* writer, } Status Counter::WriteAsPrometheus(PrometheusWriter* writer, const string& prefix) const { - prototype_->WriteFields(writer, prefix); - + prototype_->WriteHelpAndType(writer, prefix); writer->WriteEntry(Substitute("$0$1{unit_type=\"$2\"} $3\n", prefix, prototype_->name(), MetricUnit::Name(prototype_->unit()), value())); - return Status::OK(); } @@ -1096,19 +1095,16 @@ Status Histogram::WriteAsJson(JsonWriter* writer, } Status Histogram::WriteAsPrometheus(PrometheusWriter* writer, const string& prefix) const { - prototype_->WriteFields(writer, prefix); - string output = ""; - MetricJsonOptions opts; // Snapshot is taken to preserve the consistency across metrics and // requirements given by Prometheus. The value for the _bucket in +Inf // quantile needs to be equal to the total _count HistogramSnapshotPB snapshot; - RETURN_NOT_OK(GetHistogramSnapshotPB(&snapshot, opts)); + RETURN_NOT_OK(GetHistogramSnapshotPB(&snapshot, {})); - output = Substitute("$0$1$2{unit_type=\"$3\", le=\"0.75\"} $4\n", - prefix, prototype_->name(), "_bucket", - MetricUnit::Name(prototype_->unit()), - snapshot.percentile_75()); + auto output = Substitute("$0$1$2{unit_type=\"$3\", le=\"0.75\"} $4\n", + prefix, prototype_->name(), "_bucket", + MetricUnit::Name(prototype_->unit()), + snapshot.percentile_75()); SubstituteAndAppend(&output, "$0$1$2{unit_type=\"$3\", le=\"0.95\"} $4\n", prefix, prototype_->name(), "_bucket", @@ -1145,6 +1141,7 @@ Status Histogram::WriteAsPrometheus(PrometheusWriter* writer, const string& pref MetricUnit::Name(prototype_->unit()), snapshot.total_count()); + prototype_->WriteHelpAndType(writer, prefix); writer->WriteEntry(output); return Status::OK(); diff --git a/src/kudu/util/metrics.h b/src/kudu/util/metrics.h index e4b6b27c5..7efc7e471 100644 --- a/src/kudu/util/metrics.h +++ b/src/kudu/util/metrics.h @@ -583,8 +583,9 @@ class MetricPrototype { void WriteFields(JsonWriter* writer, const MetricJsonOptions& opts) const; - void WriteFields(PrometheusWriter* writer, const std::string& prefix) const; - + // Write TYPE and HELP meta-info for a metric in Prometheus format. + virtual void WriteHelpAndType(PrometheusWriter* writer, + const std::string& prefix) const; protected: explicit MetricPrototype(CtorArgs args); virtual ~MetricPrototype() = default; @@ -1013,6 +1014,14 @@ class GaugePrototype : public MetricPrototype { return MetricType::kGauge; } + void WriteHelpAndType(PrometheusWriter* writer, + const std::string& prefix) const override { + if constexpr (std::is_arithmetic_v<T>) { + // Non-arithmetic gauges aren't supported by Prometheus. + MetricPrototype::WriteHelpAndType(writer, prefix); + } + } + private: DISALLOW_COPY_AND_ASSIGN(GaugePrototype); }; @@ -1048,11 +1057,10 @@ class StringGauge : public Gauge { return false; } void MergeFrom(const scoped_refptr<Metric>& other) override; - + Status WriteAsPrometheus(PrometheusWriter* w, const std::string& prefix) const override; protected: FRIEND_TEST(MetricsTest, SimpleStringGaugeForMergeTest); - FRIEND_TEST(MetricsTest, StringGaugePrometheusTest); - Status WriteAsPrometheus(PrometheusWriter* w, const std::string& prefix) const override; + FRIEND_TEST(MetricsTest, StringGaugeForPrometheus); void WriteValue(JsonWriter* writer) const override; void WriteValue(PrometheusWriter* writer, const std::string& prefix) const override; void FillUniqueValuesUnlocked(); @@ -1092,6 +1100,30 @@ class MeanGauge : public Gauge { DISALLOW_COPY_AND_ASSIGN(MeanGauge); }; +// A helper function for writing a gauge's value in Prometheus format. +template<typename T> +void WriteValuePrometheus(PrometheusWriter* writer, + const std::string& prefix, + const char* proto_name, + const char* unit_name, + const T& value) { + static constexpr const char* const kFmt = "$0$1{unit_type=\"$2\"} $3\n"; + + if constexpr (!std::is_arithmetic_v<T>) { + // Non-arithmetic gauges aren't supported by Prometheus. + return; + } + + // For a boolean gauge, convert false/true to 0/1 for Prometheus. + if constexpr (std::is_same_v<T, bool>) { + return writer->WriteEntry( + strings::Substitute(kFmt, prefix, proto_name, unit_name, value ? 1 : 0)); + } else { + return writer->WriteEntry( + strings::Substitute(kFmt, prefix, proto_name, unit_name, value)); + } +} + // Lock-free implementation for types that are convertible to/from int64_t. template <typename T> class AtomicGauge : public Gauge { @@ -1162,23 +1194,11 @@ class AtomicGauge : public Gauge { } void WriteValue(PrometheusWriter* writer,const std::string& prefix) const override { - std::string output = ""; - - // If Boolean Gauge, convert false/true to 0/1 for Prometheus - if constexpr (std::is_same_v<T, bool> ) { - int check = 0; - if (value() == true) { - check = 1; - } else { - check = 0; - } - output = strings::Substitute("$0$1{unit_type=\"$2\"} $3\n", prefix, prototype_->name(), - MetricUnit::Name(prototype_->unit()), check); - } else { - output = strings::Substitute("$0$1{unit_type=\"$2\"} $3\n", prefix, prototype_->name(), - MetricUnit::Name(prototype_->unit()), value()); - } - writer->WriteEntry(output); + return WriteValuePrometheus(writer, + prefix, + prototype_->name(), + MetricUnit::Name(prototype_->unit()), + value()); } private: AtomicInt<int64_t> value_; @@ -1273,23 +1293,13 @@ class FunctionGauge : public Gauge { } void WriteValue(PrometheusWriter* writer, const std::string& prefix) const override { - std::string output = ""; - // If Boolean Gauge, convert false/true to 0/1 for Prometheus - if constexpr (std::is_same_v<T, bool> ) { - int check = 0; - if (value() == true) { - check = 1; - } else { - check = 0; - } - output = strings::Substitute("$0$1{unit_type=\"$2\"} $3\n", prefix, prototype_->name(), - MetricUnit::Name(prototype_->unit()), check); - } else { - output = strings::Substitute("$0$1{unit_type=\"$2\"} $3\n", prefix, prototype_->name(), - MetricUnit::Name(prototype_->unit()), value()); - } - writer->WriteEntry(output); + return WriteValuePrometheus(writer, + prefix, + prototype_->name(), + MetricUnit::Name(prototype_->unit()), + value()); } + // Reset this FunctionGauge to return a specific value. // This should be used during destruction. If you want a settable // Gauge, use a normal Gauge instead of a FunctionGauge.