[jira] [Commented] (FLINK-40648) PrometheusReporter drops all metrics of a process when two metric group variables sanitise to the same label name

["Martijn Visser (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/FLINK-40648?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18115034#comment-18115034 ] 

Martijn Visser commented on FLINK-40648:
----------------------------------------

The maintained Prometheus client already does this check. client_java 1.x Labels.validateNames compares the sanitised names and throws "a-b: duplicate label name" on exactly this input, and Micrometer rejects the meter at registration. Nobody renames or suffixes.

We shouldn't throw though. FLINK-7692 settled that metrics don't fail the program, and FLINK-11251 treated a silently changed exported name as a backport. The closest thing we already have is FLINK-39127, which logs and counts collisions best-effort without blocking registration.

So: refuse the one metric, warn once per distinct collision, keep exporting the rest. Making the reuse key (scopedName, sanitised dimensionKeys) instead of the scoped name alone covers the metric-name case and FLINK-32649 in the same change. notifyOfRemovedMetric needs a guard at the same time, since it dereferences the map entry unconditionally and would NPE once a metric can be refused.

> PrometheusReporter drops all metrics of a process when two metric group variables sanitise to the same label name
> -----------------------------------------------------------------------------------------------------------------
>
>                 Key: FLINK-40648
>                 URL: https://issues.apache.org/jira/browse/FLINK-40648
>             Project: Flink
>          Issue Type: Bug
>          Components: Runtime / Metrics
>    Affects Versions: 2.3.0, 2.2.1, 1.20.5, 2.4.0
>            Reporter: Martijn Visser
>            Priority: Critical
>
> AbstractPrometheusReporter.notifyOfAddedMetric builds the Prometheus label names by filtering every metric group variable through [^a-zA-Z0-9:_] -> _ and adding each result to dimensionKeys, without checking whether two keys produced the same name.
> metricGroup.addGroup("a.b", "v1").addGroup("a-b", "v2") gives the variables <a.b> and <a-b>, both of which filter to a_b, and the scrape then carries
> {code}
> flink_scope_dup{a_b="v1",a_b="v2",} 7.0
> {code}
> Prometheus 3.11.2 scraping that body:
> {code}
> health   = down
> lastError= label name "a_b" is not unique: invalid sample
> {code}
> The rejection is in tsdb/head_append.go, and scrape.go has no case for ErrInvalidSample, so it breaks out of the sample loop. The scrape is abandoned rather than the one sample skipped, which means every other metric from that TaskManager or JobManager is lost too and the target goes down. Verified with a body holding the colliding metric plus one valid metric: both come back empty and up is 0.
> Only a user-supplied addGroup key can trigger this. Flink's own variables are all [a-z0-9_] already.
> Two metrics named a.b and a-b in one group collide the same way one level up. There the second metric reuses the first one's collector, so the series shows whichever registered last under the first one's help string. That is the same defect FLINK-32649 reports, since collectorsWithCountByMetricName is keyed on the scoped name without the label names.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)