Skip to content

Make Prometheus latency output consistent between LatencyRecorder and its multi dimension version - #3549

Open
chenBright wants to merge 1 commit into
apache:masterfrom
chenBright:fix_latency_text_format
Open

chenBright wants to merge 1 commit into
apache:masterfrom
chenBright:fix_latency_text_format

Conversation

@chenBright

Copy link
Copy Markdown
Contributor

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:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Add coverage for all configurable quantiles and assert that the legacy unlabeled average series is absent.

Pull request overview

This PR standardizes Prometheus latency metrics by separating average latency and using fractional quantile labels.

Changes:

  • Adds _latency_average gauges.
  • Normalizes multi-dimensional quantiles.
  • Updates exporter tests.
File summaries
File Summary
test/brpc_prometheus_metrics_unittest.cpp Adds output assertions; coverage is missing for some quantiles and removal of the legacy average series.
src/bvar/multi_dimension.h Updates quantile formatting parameters to double.
src/bvar/multi_dimension_inl.h Emits fractional quantiles and separate average metrics.
src/brpc/builtin/prometheus_metrics_service.cpp Emits single-dimensional average gauges.
Review details

Suppressed comments (2)

test/brpc_prometheus_metrics_unittest.cpp:110

  • These assertions cover p3 and the fixed 0.999/0.9999 tails, but not the other configurable percentiles. Since the changed loop formats p1, p2, and p3, a regression that still emits quantile="80" or quantile="90" would pass this test; add checks for both their fractional forms and their integer forms.
    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\""));

test/brpc_prometheus_metrics_unittest.cpp:112

  • The new checks prove that a quantile series and the new average metric exist, but they do not prove that the old unlabeled mlat_latency{label1=...,label2=...} average was removed. A regression that emits both formats would still pass while reintroducing the aggregation bug described in the PR; add a negative assertion for that exact old sample.
    ASSERT_NE(std::string::npos, res.find("mlat_latency{label1=\"val1\",label2=\"val2\","
                                          "quantile=\"0.99\"}"));
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Moderate performance and regression-test issues remain, along with a metric-name documentation mismatch.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/brpc/builtin/prometheus_metrics_service.cpp:189

  • The public metric emitted here is <name>_avg_latency, while the PR description calls it N_latency_average. Please align the description with the implementation (_avg_latency), since this spelling is what Prometheus users must query.
    *_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';

test/brpc_prometheus_metrics_unittest.cpp:107

  • These checks do not prove that the multi-dimensional emitter converted all of its quantile labels: the searches for 0.99/0.999/0.9999 are unscoped and can match the single-dimensional emitter, while 0.8 and 0.9 are not checked at all. Scope assertions to the mlat_latency label set and cover all five quantiles so a regression in this changed path cannot pass unnoticed.
    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\""));
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment on lines +309 to +313
for (auto &label_name : label_names) {
LatencyRecorder* bvar = get_stats_impl(label_name);
if (nullptr == bvar) {
continue;
}
Comment on lines +111 to +112
ASSERT_NE(std::string::npos, res.find("mlat_latency{label1=\"val1\",label2=\"val2\","
"quantile=\"0.99\"}"));
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

为什么brpc_metrics接口不返回服务的平均latency? brpc_metrics里面缺少单独的average统计

2 participants