Repository navigation
[fix](metrics) Escape label values in the Prometheus output of FE - #69837
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28655 ms |
Contributor
TPC-DS: Total hot run time: 151529 ms |
Contributor
ClickBench: Total hot run time: 24.39 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: The Prometheus text format requires a backslash, a
double quote and a line feed inside a label value to be written as
`\\`, `\"` and `\n`. `PrometheusMetricVisitor` wrote label values as
they were. A table name may contain any of the three characters
(`enable_unicode_name_support` is on by default), and in cloud mode the
Master FE exports four per-table series labeled with the table name
(`doris_fe_table_data_size` and the rowset, segment and row counts).
The series of one such table left the `/metrics` response of the
Master outside the text format (`promtool check metrics` rejects each
of the three characters written raw), and Prometheus keeps no sample of
a scrape it cannot parse.
Every label value that is not a literal now goes through one function,
`escapeLabelValue`: the labels of registered metrics and of both kinds
of histograms, the per-table series, the workload group series and the
JVM GC series. Ordinary names are written as before. The coming
monitoring of table streams and IVM materialized views labels series
with their names and will render them through the same function.
### Release note
In cloud mode, a table whose name contains a backslash, a double quote
or a line feed no longer makes the FE `/metrics` output unparsable for
Prometheus.
### Check List (For Author)
- Test: Unit Test
- `MetricsTest`: a gauge and both kinds of histograms whose label
value holds all three characters, and the cloud per-table series
of such a table; the existing cases pin the unchanged output of
ordinary names.
- Behavior changed: Yes. Label values that contain a backslash, a
double quote or a line feed are escaped in the Prometheus output;
nothing else changes.
- Does this need documentation: No
Co-Authored-By: Claude Opus 5.5 <[email protected]>
morningman
force-pushed
the
changelog-monitor-t8
branch
from
October 31, 2026 01:51
423f437 to
8a9f618
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary
The label escaping covers all changed Prometheus rendering paths and correctly handles backslashes, quotes, and line feeds. It also introduces one P1 regression: table warm-up jobs register a null source-cluster label, and the new helper dereferences that null while rendering the public /metrics endpoint. The inline comment below describes the complete trigger and fix direction.
I traced the endpoint from MetricsAction through MetricRepo.getMetric, the metric registry, histogram dispatch, cloud table statistics, workload-group output, and the warm-up lifecycle. The visitor is otherwise stateless and adds no lock or concurrency changes; cloud statistics are published with non-null catalog names, and the added test restores its mutated global state. The histogram delimiter behavior considered during review predates this patch and is not a new defect. There are no existing blocking inline findings and no additional user focus items.
Testing was inspected but not executed, as required by the static-only review contract. The review converged after 2 bounded rounds with complete changed-file coverage and one verified finding.
Review converged after 2 round(s).
| List<MetricLabel> labels = metric.getLabels(); | ||
| if (!labels.isEmpty()) { | ||
| sb.append("{"); | ||
| List<String> labelStrs = labels.stream().map(l -> l.getKey() + "=\"" + l.getValue() |
Contributor
There was a problem hiding this comment.
[P1] Null source cluster in legacy table warm-up metrics crashes /metrics
Table warm-up jobs use a constructor that sets srcClusterName to null. When such a job is cancelled or times out, updateClusterWarmUpJobLastFinishTime registers src_cluster_name=null. The new visit() call invokes escapeLabelValue(null), whose value.replace(...) throws; MetricRepo.getMetric and MetricsAction do not catch it, so the entire public /metrics response fails after a normal table-job cancellation. Please preserve the old null rendering or normalize nullable label values before escaping.
Contributor
Author
There was a problem hiding this comment.
Fixed in 4ddeac2: escapeLabelValue now writes a null value as null, as the concatenation without escaping did, so every value without a backslash, a double quote or a line feed is written exactly as before. MetricsTest#testPrometheusVisitorWritesNullLabelValue writes a gauge and a labeled histogram with a null label value; without the fix it fails with a NullPointerException.
The trigger is wider than a cancellation. runPendingJob calls updateClusterWarmUpJobLatestStartTime with the null srcClusterName each time a table warm-up job starts, and neither per-job time series is ever removed from the registry. So in cloud mode the first table warm-up job to start would have made every scrape of the Master FE's /metrics fail until the FE restarted. The other producers are covered too: the registry already accepts a null label value (computeLabelId maps it to an empty string), and the fix sits in the one function every label value goes through.
Contributor
TPC-H: Total hot run time: 28354 ms |
Contributor
TPC-DS: Total hot run time: 150902 ms |
6 of 13 tasks
Contributor
ClickBench: Total hot run time: 24.36 s |
…put of FE ### What problem does this PR solve? Issue Number: apache#69907 Related PR: apache#69837 Problem Summary: `escapeLabelValue` called `replace` on every label value, so a null one threw a NullPointerException, and nothing on the way to `/metrics` catches it: `DorisMetricRegistry.accept`, `MetricRepo.getMetric` and `MetricsAction` let it through, and the whole response fails. Label values can be null: a table warm-up job (`WARM UP COMPUTE GROUP <dst> WITH TABLE ...`) has no source cluster, so `CloudWarmUpJob` keeps its `srcClusterName` null, and the job registers `doris_fe_file_cache_warm_up_job_latest_start_time` each time it starts and `doris_fe_file_cache_warm_up_job_last_finish_time` when it is cancelled, both with `src_cluster_name` set to null. Neither series is ever removed, so in cloud mode the first table warm-up job to start made every scrape of the Master FE's `/metrics` fail until the FE restarted. The registry itself already accepts a null label value (`computeLabelId` maps it to an empty string). Before label values were escaped, the string concatenation wrote it as `null`. `escapeLabelValue` now writes a null value as `null`, so the output is again unchanged for every value without a backslash, a double quote or a line feed. ### Release note None ### Check List (For Author) - Test: Unit Test - `MetricsTest#testPrometheusVisitorWritesNullLabelValue`: a gauge and a labeled histogram with a null label value are written with `null`; without this fix the case fails with a NullPointerException. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed the complete two-file PR and traced all changed Prometheus serialization paths through MetricRepo, DorisMetricRegistry, cloud warm-up/table-stat producers, workload groups, and the public metrics endpoint. The helper correctly escapes backslashes, quotes, and line feeds while preserving the previous null rendering; the added tests cover generic labels, both histogram forms, null labels, and all four cloud table-stat series. The prior P1 null-label thread is fixed in this head and is not reposted. No new blocking or non-blocking finding was substantiated. User-provided review focus: no additional focus points were supplied. Review was static-only; builds and tests were inspected but not executed. Critical checkpoints: the change is small and stateless, introduces no new concurrency, lifecycle, configuration, persistence, or protocol path, preserves metric identity for existing values, and has no remaining coverage gap in the changed files.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28887 ms |
Contributor
TPC-DS: Total hot run time: 150639 ms |
Contributor
ClickBench: Total hot run time: 24.34 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
morrySnow
approved these changes
Nov 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: #69907
Related PR: None
Problem Summary:
In short: the FE writes metric label values into its Prometheus output without escaping them. A table name may contain a backslash, a double quote or a line feed, and in cloud mode the Master FE labels four per-table series with the table name, so a single such table leaves the Master's whole
/metricsresponse outside the text format, and Prometheus keeps no sample of a scrape it cannot parse. This PR escapes every label value that is not a literal through one function, as the format requires. Ordinary names are written exactly as before.Background
In the Prometheus text format a label value is written between double quotes, and a backslash, a double quote or a line feed inside it must be written as
\\,\"and\n; no other character is escaped. A scrape whose response does not parse fails as a whole.With
enable_unicode_name_support, which is on by default, a table name may contain all three characters:FeNameFormat.UNICODE_TABLE_NAME_REGEXonly requires the last character not to be white space. A local cluster with default settings accepts tables named`a"b`,`a\b`and one with a line feed in the middle. The names of databases, users, workload groups, jobs and catalogs keep their restricted patterns.The problem, and what it cost
PrometheusMetricVisitorconcatenates label values as they are, in every place that writes one: the labels of registered metrics (visit), both kinds of histograms, the cloud per-table series (visitCloudTableStats), the workload group series and the JVM GC series.doris_fe_table_data_size,doris_fe_table_rowset_count,doris_fe_table_segment_countanddoris_fe_table_row_countfor up toprom_output_table_metrics_limit(10000) tables, labeled withdb_nameandtable_name. One table whose name holds one of the three characters makes the response invalid, and every metric of the Master FE is lost while that table is exported.How this PR fixes it
escapeLabelValuewrites a backslash, a double quote and a line feed as\\,\"and\nand leaves every other character alone, which is the full set the format requires.visit, the database and table names of the per-table series (escaped once per table, not once per series), the workload group name and the GC name. The JVM GC and workload group names cannot hold these characters today; they go through the same function so that no label value in the class is written raw.String.replacereturns the string itself when nothing matches).null, as the concatenation without escaping wrote it. Label values can be null: a table warm-up job has no source cluster, so the series of its start and finish times carry a nullsrc_cluster_name.Results
What the cloud per-table series is written as for each table name, and what
promtool check metrics(Prometheus v3.2.0) says about it:t1table_name="t1"table_name="t1"a"btable_name="a"b"unexpected end of label value "a"table_name="a\"b"a\btable_name="a\b"invalid escape sequence '\b'table_name="a\\b"a\table_name="a\"label value "a\"} 100" contains unescaped new-linetable_name="a\\"a, line feed,blabel value "a" contains unescaped new-linetable_name="a\nb"A parser that follows the format turns each escaped value back into the table name itself.
Classes, and how they call each other
PrometheusMetricVisitor: the new privateescapeLabelValue, called wherever a label value is written.MetricsTest:testPrometheusVisitorEscapesLabelValues(a gauge with an escaped and a plain label, and both kinds of histograms),testCloudTableStatsEscapeTableName(the four per-table series, with a mockedCloudTabletStatMgr) andtestPrometheusVisitorWritesNullLabelValue(a gauge and a labeled histogram with a null label value).Not changed: the JSON output (
type=json), which builds its own strings and would need JSON escaping instead; the core output (type=core), which writes no labels; the metric names and the# HELPtexts, which are literals.Release note
In cloud mode, a table whose name contains a backslash, a double quote or a line feed no longer makes the FE
/metricsoutput unparsable for Prometheus.Check List (For Author)
Test
Unit test:
MetricsTest, 21 cases including the three new ones; the two escaping cases fail on the code before this PR, and the null case fails with a NullPointerException without the null handling. Manual: the three table names above created on a local shared-nothing cluster with default settings, and the samples in the table checked withpromtool check metrics.Behavior changed:
A label value that contains a backslash, a double quote or a line feed is escaped in the Prometheus output; every other output is unchanged.
Does this need documentation?
Check List (For Reviewer who merge this PR)