You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "chenBright (via GitHub)" <gi...@apache.org> on 2026/09/16 17:06:36 UTC
[PR] Make Prometheus latency output consistent between LatencyRecorder and its multi dimension version (brpc)
chenBright opened a new pull request, #3549:
URL: https://github.com/apache/brpc/pull/3549
### What problem does this PR solve?
Issue Number: resolve #2012, resolve #2116
Problem Summary:
`LatencyRecorder` is exported to Prometheus by two independent emitters that
disagree on the latency format, and both emit a `quantile` label that is not
spec compliant:
- `PrometheusMetricsDumper::DumpLatencyRecorderSuffix` (single dimension) dumps
the average as `quantile="avg"`. Per the Prometheus exposition format the `quantile`
label value must be parsable as a float, so `avg` is illegal and makes the whole summary
unparsable for strict clients.
- The `LatencyRecorder` specialization of `MultiDimension::dump_impl` (mbvar) dumps
the average as a bare `N_latency{labels}` series, sharing the metric name with the
`N_latency{labels,quantile=...}` percentile series. Since the average carries no `quantile`
label, there is no way to filter it out, so any aggregation over `N_latency` silently mixes
the average into the percentiles. For example a Grafana panel querying
`sum by (label1) (N_latency)` or `avg(N_latency)` adds the average on top of `0.8` / `0.9` /
`0.99` / `0.999` / `0.9999`, and a legend grouped by `quantile` gets an extra unlabeled entry.
- On top of that, the mbvar quantiles are integers (`quantile="80"`, `"999"`, `"9999"`) instead
of fractions, which is also not spec compliant.
### What is changed and the side effects?
Changed:
- The average latency is dumped as a separate `N_latency_average` gauge on both paths,
instead of `quantile="avg"` (single dimension) or a bare `N_latency` series (mbvar). This
makes `N_latency` contain nothing but quantile series, so aggregating over it no longer
picks up the average. On the single dimension path `N_latency_average` is dumped before
the summary it belongs to, so the summary block stays contiguous.
- mbvar quantile labels are now fractions (`0.8` / `0.9` / `0.99` / `0.999` / `0.9999`), matching
the single dimension output. The `quantile` parameter of
`make_dump_key()` / `make_labels_kvpair_string()` is widened from `int` to `double`, and a
non-positive value means "not a quantile series".
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Make Prometheus latency output consistent between LatencyRecorder and its multi dimension version (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3549:
URL: https://github.com/apache/brpc/pull/3549
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Make Prometheus latency output consistent between LatencyRecorder and its multi dimension version (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3549:
URL: https://github.com/apache/brpc/pull/3549#discussion_r4033505431
##########
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';
Review Comment:
The PR description specifies `N_latency_average`, but this emits `N_avg_latency` (and the multi-dimensional path/tests use that name). Since this is a new exported metric name, the implementation and stated contract should agree before merging.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Make Prometheus latency output consistent between LatencyRecorder and its multi dimension version (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3549:
URL: https://github.com/apache/brpc/pull/3549#discussion_r4033467936
##########
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:
Opt in a7e6236a9bc70ccea361d2049b0b444cbaf3c7b3 .
##########
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:
fixed in a7e6236a9bc70ccea361d2049b0b444cbaf3c7b3.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Make Prometheus latency output consistent between LatencyRecorder and its multi dimension version (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3549:
URL: https://github.com/apache/brpc/pull/3549#issuecomment-5714780314
LGTM
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Make Prometheus latency output consistent between LatencyRecorder and its multi dimension version (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Make Prometheus latency output consistent between LatencyRecorder and its multi dimension version (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3549:
URL: https://github.com/apache/brpc/pull/3549#discussion_r4033525568
##########
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';
Review Comment:
The PR description has been updated.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org