Fix format of prometheus service (#2899)

ZhengweiZhu · William-Zhu · web-flow · commit b4ce61f8f493 · 2025-03-12T09:33:33.000+08:00
Fix PrometheusMetricsDumper to dump unique comment of a certain
metric name to follow the prometheus specification.
Also refine the prometheus ut case to support mvar output

Co-authored-by: Zhengwei Zhu &lt;zheng_wei36@163.com&gt;
diff --git a/BUILD.bazel b/BUILD.bazel
@@ -465,6 +465,7 @@ filegroup(
 proto_library(
     name = "brpc_idl_options_proto",
     srcs = [":brpc_idl_options_proto_srcs"],
+    strip_import_prefix = "src",
     visibility = ["//visibility:public"],
     deps = [
         "@com_google_protobuf//:descriptor_proto",
diff --git a/src/brpc/builtin/prometheus_metrics_service.cpp b/src/brpc/builtin/prometheus_metrics_service.cpp
@@ -54,6 +54,8 @@ class PrometheusMetricsDumper : public bvar::Dumper {
     }
 
     bool dump(const std::string& name, const butil::StringPiece& desc) override;
+    bool dump_mvar(const std::string& name, const butil::StringPiece& desc) override;
+    bool dump_comment(const std::string& name, const std::string& type) override;
 
 private:
     DISALLOW_COPY_AND_ASSIGN(PrometheusMetricsDumper);
@@ -108,6 +110,21 @@ bool PrometheusMetricsDumper::dump(const std::string& name,
     return true;
 }
 
+bool PrometheusMetricsDumper::dump_mvar(const std::string& name, const butil::StringPiece& desc) {
+    if (!desc.empty() && desc[0] == '"') {
+        // there is no necessary to monitor string in prometheus
+        return true;
+    }
+    *_os << name << " " << desc << "\n";
+    return true;
+}
+
+bool PrometheusMetricsDumper::dump_comment(const std::string& name, const std::string& type) {
+    *_os << "# HELP " << name << '\n'
+         << "# TYPE " << name << " " << type << '\n';
+    return true;
+}
+
 const PrometheusMetricsDumper::SummaryItems*
 PrometheusMetricsDumper::ProcessLatencyRecorderSuffix(const butil::StringPiece& name,
                                                       const butil::StringPiece& desc) {
diff --git a/src/bvar/multi_dimension_inl.h b/src/bvar/multi_dimension_inl.h
@@ -252,7 +252,7 @@ size_t MultiDimension<T>::dump(Dumper* dumper, const DumpOptions* options) {
         bvar->describe(oss, options->quote_string);
         std::ostringstream oss_key;
         make_dump_key(oss_key, label_name);
-        if (!dumper->dump(oss_key.str(), oss.str())) {
+        if (!dumper->dump_mvar(oss_key.str(), oss.str())) {
             continue;
         }
         n++;
@@ -269,20 +269,20 @@ size_t MultiDimension<bvar::LatencyRecorder>::dump(Dumper* dumper, const DumpOpt
         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(name() + "_latency", METRIC_TYPE_GAUGE);
     for (auto &label_name : label_names) {
         bvar::LatencyRecorder* bvar = get_stats_impl(label_name);
         if (!bvar) {
             continue;
         }
 
-        // latency comment
-        if (!dumper->dump_comment(name() + "_latency", METRIC_TYPE_GAUGE)) {
-            continue;
-        }
         // latency
         std::ostringstream oss_latency_key;
         make_dump_key(oss_latency_key, label_name, "_latency");
-        if (dumper->dump(oss_latency_key.str(), std::to_string(bvar->latency()))) {
+        if (dumper->dump_mvar(oss_latency_key.str(), std::to_string(bvar->latency()))) {
             n++;
         }
         // latency_percentiles
@@ -291,53 +291,62 @@ size_t MultiDimension<bvar::LatencyRecorder>::dump(Dumper* dumper, const DumpOpt
         for (auto lp : latency_percentiles) {
             std::ostringstream oss_lp_key;
             make_dump_key(oss_lp_key, label_name, "_latency", lp);
-            if (dumper->dump(oss_lp_key.str(), std::to_string(bvar->latency_percentile(lp / 100.0)))) {
+            if (dumper->dump_mvar(oss_lp_key.str(), std::to_string(bvar->latency_percentile(lp / 100.0)))) {
                 n++;
             }
         }
         // 999
         std::ostringstream oss_p999_key;
         make_dump_key(oss_p999_key, label_name, "_latency", 999);
-        if (dumper->dump(oss_p999_key.str(), std::to_string(bvar->latency_percentile(0.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(oss_p9999_key.str(), std::to_string(bvar->latency_percentile(0.9999)))) {
+        if (dumper->dump_mvar(oss_p9999_key.str(), std::to_string(bvar->latency_percentile(0.9999)))) {
             n++;
         }
+    }
 
-        // max_latency comment
-        if (!dumper->dump_comment(name() + "_max_latency", METRIC_TYPE_GAUGE)) {
+    // max_latency comment
+    dumper->dump_comment(name() + "_max_latency", METRIC_TYPE_GAUGE);
+    for (auto &label_name : label_names) {
+        bvar::LatencyRecorder* bvar = get_stats_impl(label_name);
+        if (!bvar) {
             continue;
         }
-        // max_latency
         std::ostringstream oss_max_latency_key;
         make_dump_key(oss_max_latency_key, label_name, "_max_latency");
-        if (dumper->dump(oss_max_latency_key.str(), std::to_string(bvar->max_latency()))) {
+        if (dumper->dump_mvar(oss_max_latency_key.str(), std::to_string(bvar->max_latency()))) {
             n++;
         }
-        
-        // qps comment
-        if (!dumper->dump_comment(name() + "_qps", METRIC_TYPE_GAUGE)) {
+    }
+
+    // qps comment
+    dumper->dump_comment(name() + "_qps", METRIC_TYPE_GAUGE);
+    for (auto &label_name : label_names) {
+        bvar::LatencyRecorder* bvar = get_stats_impl(label_name);
+        if (!bvar) {
             continue;
         }
-        // qps
         std::ostringstream oss_qps_key;
         make_dump_key(oss_qps_key, label_name, "_qps");
-        if (dumper->dump(oss_qps_key.str(), std::to_string(bvar->qps()))) {
+        if (dumper->dump_mvar(oss_qps_key.str(), std::to_string(bvar->qps()))) {
             n++;
         }
+    }
 
-        // qps comment
-        if (!dumper->dump_comment(name() + "_count", METRIC_TYPE_COUNTER)) {
+    // count comment
+    dumper->dump_comment(name() + "_count", METRIC_TYPE_COUNTER);
+    for (auto &label_name : label_names) {
+        bvar::LatencyRecorder* bvar = get_stats_impl(label_name);
+        if (!bvar) {
             continue;
         }
-        // count
         std::ostringstream oss_count_key;
         make_dump_key(oss_count_key, label_name, "_count");
-        if (dumper->dump(oss_count_key.str(), std::to_string(bvar->count()))) {
+        if (dumper->dump_mvar(oss_count_key.str(), std::to_string(bvar->count()))) {
             n++;
         }
     }
diff --git a/src/bvar/variable.h b/src/bvar/variable.h
@@ -53,6 +53,12 @@ class Dumper {
     virtual ~Dumper() { }
     virtual bool dump(const std::string& name,
                       const butil::StringPiece& description) = 0;
+    // Only for dumping value of multiple dimension var to prometheus service
+    virtual bool dump_mvar(const std::string& name,
+                           const butil::StringPiece& description) {
+        return true;
+    }
+    // Only for dumping comment of multiple dimension var to prometheus service
     virtual bool dump_comment(const std::string&, const std::string& /*type*/) {
         return true;
     }
diff --git a/test/BUILD.bazel b/test/BUILD.bazel
@@ -154,11 +154,12 @@ proto_library(
         [
             "*.proto",
         ],
-        exclude = [
-            "echo.proto",
-        ],
     ),
+    strip_import_prefix = "/test",
     visibility = ["//visibility:public"],
+    deps = [
+        "//:brpc_idl_options_proto",
+    ]
 )
 
 cc_proto_library(
@@ -241,6 +242,23 @@ cc_test(
     ],
 )
 
+cc_test(
+    name = "brpc_prometheus_test",
+    srcs = glob(
+        [
+            "brpc_prometheus_*_unittest.cpp",
+        ],
+    ),
+    copts = COPTS,
+    deps = [
+        ":cc_test_proto",
+        ":sstream_workaround",
+        "//:brpc",
+        "@com_google_googletest//:gtest",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 refresh_compile_commands(
     name = "brpc_test_compdb",
     # Specify the targets of interest.
diff --git a/test/brpc_channel_unittest.cpp b/test/brpc_channel_unittest.cpp
@@ -40,11 +40,7 @@
 #include "brpc/selective_channel.h"
 #include "brpc/socket_map.h"
 #include "brpc/controller.h"
-#if BAZEL_TEST
-#include "test/echo.pb.h"
-#else
 #include "echo.pb.h"
-#endif   // BAZEL_TEST
 #include "brpc/options.pb.h"
 
 namespace brpc {
diff --git a/test/brpc_prometheus_metrics_unittest.cpp b/test/brpc_prometheus_metrics_unittest.cpp
@@ -23,6 +23,7 @@
 #include "brpc/controller.h"
 #include "butil/strings/string_piece.h"
 #include "echo.pb.h"
+#include "bvar/multi_dimension.h"
 
 int main(int argc, char* argv[]) {
     testing::InitGoogleTest(&argc, argv);
@@ -45,7 +46,13 @@ enum STATE {
     HELP = 0,
     TYPE,
     GAUGE,
-    SUMMARY
+    SUMMARY,
+    COUNTER,
+    // When meets a line with a gauge/counter with labels, we have no
+    // idea the next line is a new HELP or the same gauge/counter just
+    // with different labels
+    HELP_OR_GAUGE,
+    HELP_OR_COUNTER,
 };
 
 TEST(PrometheusMetrics, sanity) {
@@ -54,10 +61,22 @@ TEST(PrometheusMetrics, sanity) {
     ASSERT_EQ(0, server.AddService(&echo_svc, brpc::SERVER_DOESNT_OWN_SERVICE));
     ASSERT_EQ(0, server.Start("127.0.0.1:8614", NULL));
 
-    brpc::Server server2;
-    DummyEchoServiceImpl echo_svc2;
-    ASSERT_EQ(0, server2.AddService(&echo_svc2, brpc::SERVER_DOESNT_OWN_SERVICE));
-    ASSERT_EQ(0, server2.Start("127.0.0.1:8615", NULL));
+    const std::list<std::string> labels = {"label1", "label2"};
+    bvar::MultiDimension<bvar::Adder<uint32_t> > my_madder("madder", labels);
+    bvar::Adder<uint32_t>* my_adder1 = my_madder.get_stats({"val1", "val2"});
+    ASSERT_TRUE(my_adder1);
+    *my_adder1 << 1 << 2;
+    bvar::Adder<uint32_t>* my_adder2 = my_madder.get_stats({"val2", "val3"});
+    ASSERT_TRUE(my_adder1);
+    *my_adder2 << 3 << 4;
+
+    bvar::MultiDimension<bvar::LatencyRecorder > my_mlat("mlat", labels);
+    bvar::LatencyRecorder* my_lat1 = my_mlat.get_stats({"val1", "val2"});
+    ASSERT_TRUE(my_lat1);
+    *my_lat1 << 1 << 2;
+    bvar::LatencyRecorder* my_lat2 = my_mlat.get_stats({"val2", "val3"});
+    ASSERT_TRUE(my_lat2);
+    *my_lat2 << 3 << 4;
 
     brpc::Channel channel;
     brpc::ChannelOptions channel_opts;
@@ -68,19 +87,22 @@ TEST(PrometheusMetrics, sanity) {
     channel.CallMethod(NULL, &cntl, NULL, NULL, NULL);
     ASSERT_FALSE(cntl.Failed());
     std::string res = cntl.response_attachment().to_string();
-
+    LOG(INFO) << "output:\n" << res;
     size_t start_pos = 0;
     size_t end_pos = 0;
+    size_t label_start = 0;
     STATE state = HELP;
     char name_help[128];
     char name_type[128];
     char type[16];
     int matched = 0;
-    int gauge_num = 0;
+    int num = 0;
     bool summary_sum_gathered = false;
     bool summary_count_gathered = false;
     bool has_ever_summary = false;
     bool has_ever_gauge = false;
+    bool has_ever_counter = false; // brought in by mvar latency recorder
+    std::unordered_set<std::string> metric_name_set;
 
     while ((end_pos = res.find('\n', start_pos)) != butil::StringPiece::npos) {
         res[end_pos] = '\0';       // safe;
@@ -98,21 +120,52 @@ TEST(PrometheusMetrics, sanity) {
                     state = GAUGE;
                 } else if (strcmp(type, "summary") == 0) {
                     state = SUMMARY;
+                } else if (strcmp(type, "counter") == 0) {
+                    state = COUNTER;
                 } else {
-                    ASSERT_TRUE(false);
+                    ASSERT_TRUE(false) << "invalid type: " << type;
                 }
+                ASSERT_EQ(0, metric_name_set.count(name_type)) << "second TYPE line for metric name "
+                    << name_type;
+                metric_name_set.insert(name_help);
                 break;
+            case HELP_OR_GAUGE:
+            case HELP_OR_COUNTER:
+                matched = sscanf(res.data() + start_pos, "# HELP %s", name_help);
+                // Try to figure out current line is a new COMMENT or not
+                if (matched == 1) {
+                    state = HELP;
+                } else {
+                    state = state == HELP_OR_GAUGE ? GAUGE : COUNTER;
+                }
+                res[end_pos] = '\n'; // revert to original
+                continue; // do not jump to next line
             case GAUGE:
-                matched = sscanf(res.data() + start_pos, "%s %d", name_type, &gauge_num);
+            case COUNTER:
+                matched = sscanf(res.data() + start_pos, "%s %d", name_type, &num);
                 ASSERT_EQ(2, matched);
-                ASSERT_STREQ(name_type, name_help);
-                state = HELP;
-                has_ever_gauge = true;
+                if (state == GAUGE) {
+                    has_ever_gauge = true;
+                }
+                if (state == COUNTER) {
+                    has_ever_counter = true;
+                }
+                label_start = butil::StringPiece(name_type).find("{");
+                if (label_start == strlen(name_help)) { // mvar
+                    ASSERT_EQ(name_type[strlen(name_type) - 1], '}');
+                    ASSERT_TRUE(strncmp(name_type, name_help, strlen(name_help)) == 0);
+                    state = state == GAUGE ? HELP_OR_GAUGE : HELP_OR_COUNTER;
+                } else if (label_start == butil::StringPiece::npos) { // var
+                    ASSERT_STREQ(name_type, name_help);
+                    state = HELP;
+                } else { // invalid
+                    ASSERT_TRUE(false);
+                }
                 break;
             case SUMMARY:
                 if (butil::StringPiece(res.data() + start_pos, end_pos - start_pos).find("quantile=")
                         == butil::StringPiece::npos) {
-                    matched = sscanf(res.data() + start_pos, "%s %d", name_type, &gauge_num);
+                    matched = sscanf(res.data() + start_pos, "%s %d", name_type, &num);
                     ASSERT_EQ(2, matched);
                     ASSERT_TRUE(strncmp(name_type, name_help, strlen(name_help)) == 0);
                     if (butil::StringPiece(name_type).ends_with("_sum")) {
@@ -138,7 +191,7 @@ TEST(PrometheusMetrics, sanity) {
         }
         start_pos = end_pos + 1;
     }
-    ASSERT_TRUE(has_ever_gauge && has_ever_summary);
+    ASSERT_TRUE(has_ever_gauge && has_ever_summary && has_ever_counter);
     ASSERT_EQ(0, server.Stop(0));
     ASSERT_EQ(0, server.Join());
 }
diff --git a/test/iobuf_unittest.cpp b/test/iobuf_unittest.cpp
@@ -32,11 +32,7 @@
 #include <butil/fd_guard.h>
 #include <butil/errno.h>
 #include <butil/fast_rand.h>
-#if BAZEL_TEST
-#include "test/iobuf.pb.h"
-#else
 #include "iobuf.pb.h"
-#endif   // BAZEL_TEST
 
 namespace butil {
 namespace iobuf {
