morningman opened a new pull request, #69837:
URL: https://github.com/apache/doris/pull/69837
### What problem does this PR solve?
Issue Number: None
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 `/metrics`
response 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_REGEX` only
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
- `PrometheusMetricVisitor` concatenates 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.
- In cloud mode the Master FE exports `doris_fe_table_data_size`,
`doris_fe_table_rowset_count`, `doris_fe_table_segment_count` and
`doris_fe_table_row_count` for up to `prom_output_table_metrics_limit` (10000)
tables, labeled with `db_name` and `table_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.
- The coming monitoring of table streams and IVM materialized views labels
series with the names of materialized views and streams, which would run into
the same problem in both modes.
#### How this PR fixes it
- `escapeLabelValue` writes a backslash, a double quote and a line feed as
`\\`, `\"` and `\n` and leaves every other character alone, which is the full
set the format requires.
- Every label value that is not a literal goes through it: the two histogram
paths and `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.
- A name without these characters comes back as the same string, so the
output of ordinary names does not change, byte for byte, and costs no extra
allocation (`String.replace` returns the string itself when nothing matches).
#### 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:
| Table name | Before this PR | promtool | After this PR | promtool |
| --- | --- | --- | --- | --- |
| `t1` | `table_name="t1"` | OK | `table_name="t1"` | OK |
| `a"b` | `table_name="a"b"` | `unexpected end of label value "a"` |
`table_name="a\"b"` | OK |
| `a\b` | `table_name="a\b"` | `invalid escape sequence '\b'` |
`table_name="a\\b"` | OK |
| `a\` | `table_name="a\"` | `label value "a\"} 100" contains unescaped
new-line` | `table_name="a\\"` | OK |
| `a`, line feed, `b` | the sample split over two lines | `label value "a"
contains unescaped new-line` | `table_name="a\nb"` | OK |
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 private `escapeLabelValue`, called
wherever a label value is written.
- `MetricsTest`: `testPrometheusVisitorEscapesLabelValues` (a gauge with an
escaped and a plain label, and both kinds of histograms) and
`testCloudTableStatsEscapeTableName` (the four per-table series, with a mocked
`CloudTabletStatMgr`).
```
MetricsAction (/metrics) -> MetricRepo.getMetric(PrometheusMetricVisitor)
|-- visitJvm GC name -> escapeLabelValue
|-- DORIS_METRIC_REGISTER.accept -> visit labels ->
escapeLabelValue
|-- visitHistograms -> visitHistogram (both) labels ->
escapeLabelValue
|-- visitCloudTableStats db / table name -> escapeLabelValue
`-- visitWorkloadGroup group name -> escapeLabelValue
```
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 `# HELP` texts, 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 `/metrics` output unparsable for Prometheus.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [x] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
Unit test: `MetricsTest`, 20 cases including the two new ones; both new
cases fail on the code before this PR. Manual: the three table names above
created on a local shared-nothing cluster with default settings, and the
samples in the table checked with `promtool check metrics`.
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
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?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
