optimize

src/brpc/builtin/metrics_service.cpp → src/brpc/builtin/prometheus_metrics_service.cpp

@@ -20,7 +20,7 @@
 #include "brpc/controller.h"                // Controller
 #include "brpc/server.h"                    // Server
 #include "brpc/closure_guard.h"             // ClosureGuard
-#include "brpc/builtin/metrics_service.h"
+#include "brpc/builtin/prometheus_metrics_service.h"
 #include "brpc/builtin/common.h"
 #include "bvar/bvar.h"
 
@@ -39,9 +39,9 @@ namespace brpc {
 // more counter is just another gauge.
 // 2) Histogram and summary is equivalent except that histogram
 // calculates quantiles in the server side.
-class MetricsDumper : public bvar::Dumper {
+class PrometheusMetricsDumper : public bvar::Dumper {
 public:
-    explicit MetricsDumper(butil::IOBufBuilder& os,
+    explicit PrometheusMetricsDumper(butil::IOBufBuilder& os,
                            const std::string& server_prefix)
         : _os(os)
         , _server_prefix(server_prefix) {
@@ -50,7 +50,7 @@ public:
     bool dump(const std::string& name, const butil::StringPiece& desc);
 
 private:
-    DISALLOW_COPY_AND_ASSIGN(MetricsDumper);
+    DISALLOW_COPY_AND_ASSIGN(PrometheusMetricsDumper);
 
     bool ProcessLatencyRecorderSuffix(const butil::StringPiece& name,
                                       const butil::StringPiece& desc);
@@ -74,8 +74,8 @@ private:
     std::map<std::string, SummaryItems> _m;
 };
 
-bool MetricsDumper::dump(const std::string& name, const butil::StringPiece& desc) {
-    if (desc.size() > 0 && desc[0] == '"') {
+bool PrometheusMetricsDumper::dump(const std::string& name, const butil::StringPiece& desc) {
+    if (!desc.empty() && desc[0] == '"') {
         // there is no necessary to monitor string in prometheus
         return true;
     }
@@ -90,7 +90,7 @@ bool MetricsDumper::dump(const std::string& name, const butil::StringPiece& desc
     return true;
 }
 
-bool MetricsDumper::ProcessLatencyRecorderSuffix(const butil::StringPiece& name,
+bool PrometheusMetricsDumper::ProcessLatencyRecorderSuffix(const butil::StringPiece& name,
                                                            const butil::StringPiece& desc) {
     static std::string latency_names[] = {
         butil::string_printf("latency_%d", (int)bvar::FLAGS_bvar_latency_p1),
@@ -100,10 +100,11 @@ bool MetricsDumper::ProcessLatencyRecorderSuffix(const butil::StringPiece& name,
     };
     CHECK(NPERCENTILES == sizeof(latency_names) / sizeof(std::string));
 
+    if (name.starts_with(_server_prefix)) {
         int i = 0;
         butil::StringPiece key(name);
         for (; i < NPERCENTILES; ++i) {
-        if (name.starts_with(_server_prefix) && name.ends_with(latency_names[i])) {
+            if (key.ends_with(latency_names[i])) {
                 key.remove_suffix(latency_names[i].size() + 1); /* 1 is sizeof('_') */
                 _m[key.as_string()].latency_percentiles[i] = desc.as_string();
                 break;
@@ -137,25 +138,25 @@ bool MetricsDumper::ProcessLatencyRecorderSuffix(const butil::StringPiece& name,
             return true;
         }
         // Get the average of latency in recent window size
-    if (name.starts_with(_server_prefix) && name.ends_with("latency")) {
+        if (key.ends_with("latency")) {
             key.remove_suffix(8 /* sizeof("latency") + sizeof('_') */);
             _m[key.as_string()].latency_avg = desc.as_string();
             return true;
         }
-
-    if (name.starts_with(_server_prefix) && name.ends_with("count") &&
+        if (key.ends_with("count") &&
+                !key.ends_with("builtin_service_count") &&
                 // 'builtin_service_count' and 'connection_count' is not
                 // exposed by bvar in LatencyRecorder
-            !name.ends_with("builtin_service_count") &&
-            !name.ends_with("connection_count")) {
+                !key.ends_with("connection_count")) {
             key.remove_suffix(6 /* sizeof("count") + sizeof('_') */);
             _m[key.as_string()].count = desc.as_string();
             return true;
         }
+    }
     return false;
 }
 
-void MetricsService::default_method(::google::protobuf::RpcController* cntl_base,
+void PrometheusMetricsService::default_method(::google::protobuf::RpcController* cntl_base,
                                               const ::brpc::MetricsRequest*,
                                               ::brpc::MetricsResponse*,
                                               ::google::protobuf::Closure* done) {
@@ -163,7 +164,7 @@ void MetricsService::default_method(::google::protobuf::RpcController* cntl_base
     Controller *cntl = static_cast<Controller*>(cntl_base);
     cntl->http_response().set_content_type("text/plain");
     butil::IOBufBuilder os;
-    MetricsDumper dumper(os, _server->ServerPrefix());
+    PrometheusMetricsDumper dumper(os, _server->ServerPrefix());
     const int ndump = bvar::Variable::dump_exposed(&dumper, NULL);
     if (ndump < 0) {
         cntl->SetFailed("Fail to dump metrics");

src/brpc/builtin/metrics_service.h → src/brpc/builtin/prometheus_metrics_service.h

@@ -14,17 +14,17 @@
 
 // Authors: Jiashun Zhu(zhujiashun@bilibili.com)
 
-#ifndef BRPC_METRICS_SERVICE_H
-#define BRPC_METRICS_SERVICE_H
+#ifndef BRPC_PROMETHEUS_METRICS_SERVICE_H
+#define BRPC_PROMETHEUS_METRICS_SERVICE_H
 
 #include "brpc/builtin_service.pb.h"
 #include "brpc/server.h"
 
 namespace brpc {
 
-class MetricsService : public metrics {
+class PrometheusMetricsService : public metrics {
 public:
-    MetricsService(Server* server)
+    PrometheusMetricsService(Server* server)
         : _server(server) {}
 
     void default_method(::google::protobuf::RpcController* cntl_base,
@@ -37,4 +37,4 @@ private:
 
 } // namepace brpc
 
-#endif  // BRPC_METRICS_SERVICE_H
+#endif  // BRPC_PROMETHEUS_METRICS_SERVICE_H

src/brpc/server.h

@@ -522,7 +522,7 @@ friend class ProtobufsService;
 friend class ConnectionsService;
 friend class BadMethodService;
 friend class ServerPrivateAccessor;
-friend class MetricsService;
+friend class PrometheusMetricsService;
 friend class Controller;
 
     int AddServiceInternal(google::protobuf::Service* service,

test/brpc_prometheus_metrics_unittest.cpp

@@ -34,7 +34,7 @@ enum STATE {
     SUMMARY
 };
 
-TEST(PROMETHEUS_METRICS, sanity) {
+TEST(PrometheusMetrics, sanity) {
     brpc::Server server;
     DummyEchoServiceImpl echo_svc;
     ASSERT_EQ(0, server.AddService(&echo_svc, brpc::SERVER_DOESNT_OWN_SERVICE));
@@ -60,6 +60,8 @@ TEST(PROMETHEUS_METRICS, sanity) {
     int gauge_num = 0;
     bool summary_sum_gathered = false;
     bool summary_count_gathered = false;
+    bool has_ever_summary = false;
+    bool has_ever_gauge = false;
 
     while ((end_pos = res.find('\n', start_pos)) != butil::StringPiece::npos) {
         res[end_pos] = '\0';       // safe;
@@ -86,6 +88,7 @@ TEST(PROMETHEUS_METRICS, sanity) {
                 ASSERT_EQ(2, matched);
                 ASSERT_STREQ(name_type, name_help);
                 state = HELP;
+                has_ever_gauge = true;
                 break;
             case SUMMARY:
                 if (butil::StringPiece(res.data() + start_pos, end_pos - start_pos).find("quantile=")
@@ -106,6 +109,7 @@ TEST(PROMETHEUS_METRICS, sanity) {
                         state = HELP;
                         summary_sum_gathered = false;
                         summary_count_gathered = false;
+                        has_ever_summary = true;
                     }
                 } // else find "quantile=", just break to next line
                 break;
@@ -115,6 +119,7 @@ TEST(PROMETHEUS_METRICS, sanity) {
         }
         start_pos = end_pos + 1;
     }
+    ASSERT_TRUE(has_ever_gauge && has_ever_summary);
     ASSERT_EQ(0, server.Stop(0));
     ASSERT_EQ(0, server.Join());
 }