This is an automated email from the ASF dual-hosted git repository.
chenBright pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new e97cff7d Make Prometheus latency output consistent between
LatencyRecorder and its multi dimension version (#3549)
e97cff7d is described below
commit e97cff7d586d2c2f326557977b0025fac5f4974d
Author: Bright Chen <[email protected]>
AuthorDate: Thu Sep 17 23:13:19 2026 +0800
Make Prometheus latency output consistent between LatencyRecorder and its
multi dimension version (#3549)
* Make Prometheus latency output consistent between LatencyRecorder and its
multi dimension version
* Opt MultiDimension::dump_impl
---
src/brpc/builtin/prometheus_metrics_service.cpp | 8 +-
src/bvar/multi_dimension.h | 4 +-
src/bvar/multi_dimension_inl.h | 127 ++++++++++++++----------
test/brpc_prometheus_metrics_unittest.cpp | 34 +++++++
4 files changed, 116 insertions(+), 57 deletions(-)
diff --git a/src/brpc/builtin/prometheus_metrics_service.cpp
b/src/brpc/builtin/prometheus_metrics_service.cpp
index c02e78dd..16ffd895 100644
--- a/src/brpc/builtin/prometheus_metrics_service.cpp
+++ b/src/brpc/builtin/prometheus_metrics_service.cpp
@@ -181,6 +181,12 @@ bool PrometheusMetricsDumper::DumpLatencyRecorderSuffix(
if (!si->IsComplete()) {
return true;
}
+ // The average latency can not be a quantile series of the summary below,
+ // because the quantile label must be parsable as a float. Dump it as a
+ // separate gauge, which is the same as the multi dimension one does.
+ *_os << "# HELP " << si->metric_name << "_avg_latency" << '\n'
+ << "# TYPE " << si->metric_name << "_avg_latency gauge\n"
+ << si->metric_name << "_avg_latency " << si->latency_avg << '\n';
*_os << "# HELP " << si->metric_name << '\n'
<< "# TYPE " << si->metric_name << " summary\n"
<< si->metric_name << "{quantile=\""
@@ -198,8 +204,6 @@ bool PrometheusMetricsDumper::DumpLatencyRecorderSuffix(
<< si->latency_percentiles[4] << '\n'
<< si->metric_name << "{quantile=\"1\"} "
<< si->latency_percentiles[5] << '\n'
- << si->metric_name << "{quantile=\"avg\"} "
- << si->latency_avg << '\n'
<< si->metric_name << "_sum "
// There is no sum of latency in bvar output, just use
// average * count as approximation
diff --git a/src/bvar/multi_dimension.h b/src/bvar/multi_dimension.h
index 2eb31b80..bcf46c08 100644
--- a/src/bvar/multi_dimension.h
+++ b/src/bvar/multi_dimension.h
@@ -190,10 +190,10 @@ private:
dump_impl(Dumper* dumper, const DumpOptions* options);
void make_dump_key(std::ostream& os, const key_type& labels_value,
- const std::string& suffix = "", int quantile = 0);
+ const std::string& suffix = "", double quantile = 0);
void make_labels_kvpair_string(
- std::ostream& os, const key_type& labels_value, int quantile);
+ std::ostream& os, const key_type& labels_value, double quantile);
template <typename K>
diff --git a/src/bvar/multi_dimension_inl.h b/src/bvar/multi_dimension_inl.h
index 7ff15230..108a66b0 100644
--- a/src/bvar/multi_dimension_inl.h
+++ b/src/bvar/multi_dimension_inl.h
@@ -258,85 +258,103 @@ MultiDimension<T, KeyType, Shared>::dump_impl(Dumper*
dumper, const DumpOptions*
if (label_names.empty()) {
return 0;
}
- size_t n = 0;
- // To meet prometheus specification, we must guarantee no second TYPE line
for one metric name
-
- // latency comment
- dumper->dump_comment(this->name() + "_latency", METRIC_TYPE_GAUGE);
- for (auto &label_name : label_names) {
+ // The latency of one quantile. The quantile must be a fraction to meet
+ // prometheus specification, e.g. 0.99 for p99.
+ struct LatencyPercentile {
+ double quantile;
+ int64_t latency;
+ };
+ // All the values dumped for one label set.
+ struct DumpedStats {
+ const key_type* label_name;
+ LatencyPercentile latency_percentiles[5];
+ int64_t avg_latency;
+ int64_t max_latency;
+ int64_t qps;
+ int64_t count;
+ };
+ // Read all the values in one traversal, so that a LatencyRecorder is
looked
+ // up only once no matter how many metrics are dumped for it. Keep the
values
+ // instead of the LatencyRecorder pointers, which delete_stats() may free.
+ std::vector<DumpedStats> stats_list;
+ stats_list.reserve(label_names.size());
+ for (const auto& label_name : label_names) {
bvar::LatencyRecorder* bvar = get_stats_impl(label_name);
if (!bvar) {
continue;
}
-
- // latency
- std::ostringstream oss_latency_key;
- make_dump_key(oss_latency_key, label_name, "_latency");
- if (dumper->dump_mvar(oss_latency_key.str(),
std::to_string(bvar->latency()))) {
- n++;
+ DumpedStats stats{};
+ stats.label_name = &label_name;
+ stats.latency_percentiles[0].quantile = FLAGS_bvar_latency_p1 / 100.0;
+ stats.latency_percentiles[1].quantile = FLAGS_bvar_latency_p2 / 100.0;
+ stats.latency_percentiles[2].quantile = FLAGS_bvar_latency_p3 / 100.0;
+ stats.latency_percentiles[3].quantile = 0.999;
+ stats.latency_percentiles[4].quantile = 0.9999;
+ for (auto& lp : stats.latency_percentiles) {
+ lp.latency = bvar->latency_percentile(lp.quantile);
}
- // latency_percentiles
- // p1/p2/p3
- int latency_percentiles[3] {FLAGS_bvar_latency_p1,
FLAGS_bvar_latency_p2, FLAGS_bvar_latency_p3};
- for (auto lp : latency_percentiles) {
- std::ostringstream oss_lp_key;
- make_dump_key(oss_lp_key, label_name, "_latency", lp);
- if (dumper->dump_mvar(oss_lp_key.str(),
std::to_string(bvar->latency_percentile(lp / 100.0)))) {
+ stats.avg_latency = bvar->latency();
+ stats.max_latency = bvar->max_latency();
+ stats.qps = bvar->qps();
+ stats.count = bvar->count();
+ stats_list.push_back(stats);
+ }
+
+ size_t n = 0;
+
+ // To meet prometheus specification, we must guarantee no second TYPE line
for one metric name
+
+ // latency comment
+ dumper->dump_comment(this->name() + "_latency", METRIC_TYPE_GAUGE);
+ for (const auto& stats : stats_list) {
+ for (const auto& lp : stats.latency_percentiles) {
+ std::ostringstream oss_latency_key;
+ make_dump_key(oss_latency_key, *stats.label_name, "_latency",
lp.quantile);
+ if (dumper->dump_mvar(oss_latency_key.str(),
std::to_string(lp.latency))) {
n++;
}
}
- // 999
- std::ostringstream oss_p999_key;
- make_dump_key(oss_p999_key, label_name, "_latency", 999);
- if (dumper->dump_mvar(oss_p999_key.str(),
std::to_string(bvar->latency_percentile(0.999)))) {
- n++;
- }
- // 9999
- std::ostringstream oss_p9999_key;
- make_dump_key(oss_p9999_key, label_name, "_latency", 9999);
- if (dumper->dump_mvar(oss_p9999_key.str(),
std::to_string(bvar->latency_percentile(0.9999)))) {
+ }
+
+ // avg_latency comment
+ // The average latency has to be a separate metric rather than a series of
+ // `_latency` without a quantile label, otherwise an aggregation over
+ // `_latency` would silently mix the average into the percentiles.
+ dumper->dump_comment(this->name() + "_avg_latency", METRIC_TYPE_GAUGE);
+ for (const auto& stats : stats_list) {
+ std::ostringstream oss_avg_latency_key;
+ make_dump_key(oss_avg_latency_key, *stats.label_name, "_avg_latency");
+ if (dumper->dump_mvar(oss_avg_latency_key.str(),
std::to_string(stats.avg_latency))) {
n++;
}
}
// max_latency comment
dumper->dump_comment(this->name() + "_max_latency", METRIC_TYPE_GAUGE);
- for (auto &label_name : label_names) {
- LatencyRecorder* bvar = get_stats_impl(label_name);
- if (nullptr == bvar) {
- continue;
- }
+ for (const auto& stats : stats_list) {
std::ostringstream oss_max_latency_key;
- make_dump_key(oss_max_latency_key, label_name, "_max_latency");
- if (dumper->dump_mvar(oss_max_latency_key.str(),
std::to_string(bvar->max_latency()))) {
+ make_dump_key(oss_max_latency_key, *stats.label_name, "_max_latency");
+ if (dumper->dump_mvar(oss_max_latency_key.str(),
std::to_string(stats.max_latency))) {
n++;
}
}
// qps comment
dumper->dump_comment(this->name() + "_qps", METRIC_TYPE_GAUGE);
- for (auto &label_name : label_names) {
- LatencyRecorder* bvar = get_stats_impl(label_name);
- if (nullptr == bvar) {
- continue;
- }
+ for (const auto& stats : stats_list) {
std::ostringstream oss_qps_key;
- make_dump_key(oss_qps_key, label_name, "_qps");
- if (dumper->dump_mvar(oss_qps_key.str(), std::to_string(bvar->qps())))
{
+ make_dump_key(oss_qps_key, *stats.label_name, "_qps");
+ if (dumper->dump_mvar(oss_qps_key.str(), std::to_string(stats.qps))) {
n++;
}
}
// count comment
dumper->dump_comment(this->name() + "_count", METRIC_TYPE_COUNTER);
- for (auto &label_name : label_names) {
- LatencyRecorder* bvar = get_stats_impl(label_name);
- if (nullptr == bvar) {
- continue;
- }
+ for (const auto& stats : stats_list) {
std::ostringstream oss_count_key;
- make_dump_key(oss_count_key, label_name, "_count");
- if (dumper->dump_mvar(oss_count_key.str(),
std::to_string(bvar->count()))) {
+ make_dump_key(oss_count_key, *stats.label_name, "_count");
+ if (dumper->dump_mvar(oss_count_key.str(),
std::to_string(stats.count))) {
n++;
}
}
@@ -345,7 +363,7 @@ MultiDimension<T, KeyType, Shared>::dump_impl(Dumper*
dumper, const DumpOptions*
template <typename T, typename KeyType, bool Shared>
void MultiDimension<T, KeyType, Shared>::make_dump_key(std::ostream& os, const
key_type& labels_value,
- const std::string& suffix, int
quantile) {
+ const std::string&
suffix, double quantile) {
os << this->name();
if (!suffix.empty()) {
os << suffix;
@@ -354,8 +372,9 @@ void MultiDimension<T, KeyType,
Shared>::make_dump_key(std::ostream& os, const k
}
template <typename T, typename KeyType, bool Shared>
-void MultiDimension<T, KeyType, Shared>::make_labels_kvpair_string(
- std::ostream& os, const key_type& labels_value, int quantile) {
+void MultiDimension<T, KeyType,
Shared>::make_labels_kvpair_string(std::ostream& os,
+ const
key_type& labels_value,
+ double
quantile) {
os << "{";
auto label_key = this->_labels.cbegin();
auto label_value = labels_value.cbegin();
@@ -365,6 +384,8 @@ void MultiDimension<T, KeyType,
Shared>::make_labels_kvpair_string(
os << comma << label_key->c_str() << "=\"" << label_value->c_str() <<
"\"";
comma[0] = ',';
}
+ // The `quantile` label must be parsable as a float, so a non-positive
+ // `quantile` means "this metric is not a quantile series".
if (quantile > 0) {
os << comma << "quantile=\"" << quantile << "\"";
}
diff --git a/test/brpc_prometheus_metrics_unittest.cpp
b/test/brpc_prometheus_metrics_unittest.cpp
index 166d6f80..2edd484c 100644
--- a/test/brpc_prometheus_metrics_unittest.cpp
+++ b/test/brpc_prometheus_metrics_unittest.cpp
@@ -78,6 +78,10 @@ TEST(PrometheusMetrics, sanity) {
ASSERT_TRUE(my_lat2);
*my_lat2 << 3 << 4;
+ // Only a bvar prefixed with the server prefix is folded into a summary.
+ bvar::LatencyRecorder my_lat3("rpc_server_lat_test");
+ my_lat3 << 5 << 6;
+
brpc::Channel channel;
brpc::ChannelOptions channel_opts;
channel_opts.protocol = "http";
@@ -88,6 +92,36 @@ TEST(PrometheusMetrics, sanity) {
ASSERT_FALSE(cntl.Failed());
std::string res = cntl.response_attachment().to_string();
LOG(INFO) << "output:\n" << res;
+
+ // The average latency is a separate metric rather than a quantile series,
+ // because the quantile label must be parsable as a float.
+ ASSERT_EQ(std::string::npos, res.find("quantile=\"avg\""));
+ ASSERT_NE(std::string::npos, res.find("# TYPE mlat_avg_latency gauge\n"));
+ ASSERT_NE(std::string::npos, res.find("mlat_avg_latency{label1=\"val1\","
+ "label2=\"val2\"}"));
+ // The single dimension LatencyRecorder uses the same suffix.
+ ASSERT_NE(std::string::npos, res.find("_service_echo_avg_latency "));
+ // Quantile is a fraction rather than an integer.
+ ASSERT_NE(std::string::npos, res.find("quantile=\"0.99\""));
+ ASSERT_NE(std::string::npos, res.find("quantile=\"0.999\""));
+ ASSERT_NE(std::string::npos, res.find("quantile=\"0.9999\""));
+ ASSERT_EQ(std::string::npos, res.find("quantile=\"99\""));
+ ASSERT_EQ(std::string::npos, res.find("quantile=\"999\""));
+ ASSERT_EQ(std::string::npos, res.find("quantile=\"9999\""));
+ ASSERT_NE(std::string::npos,
res.find("mlat_latency{label1=\"val1\",label2=\"val2\","
+ "quantile=\"0.99\"}"));
+ // The average must not be dumped as a series of `_latency` as well,
otherwise
+ // an aggregation over `_latency` would still pick it up.
+ ASSERT_EQ(std::string::npos, res.find("mlat_latency{label1=\"val1\","
+ "label2=\"val2\"} "));
+ ASSERT_NE(std::string::npos, res.find("rpc_server_lat_test_count 2\n"));
+ // `_avg_latency` is dumped before the summary it belongs to.
+ size_t average_pos = res.find("# TYPE rpc_server_lat_test_avg_latency
gauge\n");
+ size_t summary_pos = res.find("# TYPE rpc_server_lat_test summary\n");
+ ASSERT_NE(std::string::npos, average_pos);
+ ASSERT_NE(std::string::npos, summary_pos);
+ ASSERT_LT(average_pos, summary_pos);
+
size_t start_pos = 0;
size_t end_pos = 0;
size_t label_start = 0;
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
