Copilot commented on code in PR #3549:
URL: https://github.com/apache/brpc/pull/3549#discussion_r4032958485
##########
test/brpc_prometheus_metrics_unittest.cpp:
##########
@@ -88,6 +92,32 @@ 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\"}"));
Review Comment:
The regression test checks that the new average series exists, but it never
verifies that the old unlabeled `mlat_latency{label1="val1",label2="val2"}`
series is gone. A future change could emit both series and still pass these
assertions, leaving aggregations to double-count the average; assert that the
bare label set is absent as well.
##########
src/bvar/multi_dimension_inl.h:
##########
@@ -269,32 +269,51 @@ MultiDimension<T, KeyType, Shared>::dump_impl(Dumper*
dumper, const DumpOptions*
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++;
- }
// latency_percentiles
// p1/p2/p3
- int latency_percentiles[3] {FLAGS_bvar_latency_p1,
FLAGS_bvar_latency_p2, FLAGS_bvar_latency_p3};
+ int latency_percentiles[3] {
+ FLAGS_bvar_latency_p1,
+ FLAGS_bvar_latency_p2,
+ FLAGS_bvar_latency_p3
+ };
for (auto lp : latency_percentiles) {
+ // Quantile must be a fraction, e.g. 0.99 for p99.
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)))) {
+ make_dump_key(oss_lp_key, label_name, "_latency", lp / 100.0);
+ int64_t latency_percentile = bvar->latency_percentile(lp / 100.0);
+ if (dumper->dump_mvar(oss_lp_key.str(),
std::to_string(latency_percentile))) {
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)))) {
+ make_dump_key(oss_p999_key, label_name, "_latency", 0.999);
+ int64_t latency_percentile = bvar->latency_percentile(0.999);
+ if (dumper->dump_mvar(oss_p999_key.str(),
std::to_string(latency_percentile))) {
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)))) {
+ make_dump_key(oss_p9999_key, label_name, "_latency", 0.9999);
+ latency_percentile = bvar->latency_percentile(0.9999);
+ if (dumper->dump_mvar(oss_p9999_key.str(),
std::to_string(latency_percentile))) {
+ n++;
+ }
+ }
+
+ // latency_average 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 (auto &label_name : label_names) {
+ LatencyRecorder* bvar = get_stats_impl(label_name);
+ if (nullptr == bvar) {
+ continue;
+ }
Review Comment:
This new block makes every scrape traverse `label_names` and call
`get_stats_impl` a second time; the latency block above already performs that
lookup for each label. For high-cardinality multi-dimensional variables this
adds an unnecessary O(N) DBD read/lookup to `/brpc_metrics`; cache the
pointers/values from the first traversal or otherwise avoid the extra pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
