Make Prometheus latency output consistent between LatencyRecorder and its multi dimension version

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

src/bvar/multi_dimension.h

@@ -190,10 +190,10 @@ class MultiDimension : public MVariable<KeyType> {
     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>

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 << "\"";
     }

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;