This is an automated email from the ASF dual-hosted git repository.
guangmingchen 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 b4ce61f8 Fix format of prometheus service (#2899)
b4ce61f8 is described below
commit b4ce61f8f4935ec79d3d9584433f8da9f05edd1c
Author: Regal <141622927+zhengwei...@users.noreply.github.com>
AuthorDate: Wed Mar 12 09:33:33 2025 +0800
Fix format of prometheus service (#2899)
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 <zheng_we...@163.com>
---
BUILD.bazel | 1 +
src/brpc/builtin/prometheus_metrics_service.cpp | 17 ++++++
src/bvar/multi_dimension_inl.h | 53 +++++++++-------
src/bvar/variable.h | 6 ++
test/BUILD.bazel | 24 +++++++-
test/brpc_channel_unittest.cpp | 4 --
test/brpc_prometheus_metrics_unittest.cpp | 81 ++++++++++++++++++++-----
test/iobuf_unittest.cpp | 4 --
8 files changed, 143 insertions(+), 47 deletions(-)
diff --git a/BUILD.bazel b/BUILD.bazel
index e383be4b..610d01d0 100644
--- 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
index 88f675bb..4c5dd590 100644
--- a/src/brpc/builtin/prometheus_metrics_service.cpp
+++ b/src/brpc/builtin/prometheus_metrics_service.cpp
@@ -54,6 +54,8 @@ public:
}
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
index d1132c79..b7ef2b2d 100644
--- 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
index ce6ff4ac..86e9cd0c 100644
--- a/src/bvar/variable.h
+++ b/src/bvar/variable.h
@@ -53,6 +53,12 @@ public:
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
index 1e0ef966..1fd937b2 100644
--- 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
index 93a4d3bf..66d1fbad 100644
--- 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
index 064c14f7..471fd40a 100644
--- 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
index 09dd17da..da36c180 100644
--- 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 {
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
