diff --git a/src/brpc/builtin/prometheus_metrics_service.cpp b/src/brpc/builtin/prometheus_metrics_service.cpp index c02e78ddf4..16ffd89515 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 2eb31b805d..bcf46c08d2 100644 --- a/src/bvar/multi_dimension.h +++ b/src/bvar/multi_dimension.h @@ -190,10 +190,10 @@ class MultiDimension : public MVariable { 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 diff --git a/src/bvar/multi_dimension_inl.h b/src/bvar/multi_dimension_inl.h index 7ff15230cb..108a66b0a0 100644 --- a/src/bvar/multi_dimension_inl.h +++ b/src/bvar/multi_dimension_inl.h @@ -258,85 +258,103 @@ MultiDimension::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 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::dump_impl(Dumper* dumper, const DumpOptions* template void MultiDimension::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::make_dump_key(std::ostream& os, const k } template -void MultiDimension::make_labels_kvpair_string( - std::ostream& os, const key_type& labels_value, int quantile) { +void MultiDimension::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::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 166d6f80a6..2edd484ca5 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;