sohurdc commented on code in PR #27165:
URL: https://github.com/apache/flink/pull/27165#discussion_r2476164596
##########
flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/AbstractPrometheusReporter.java:
##########
@@ -153,31 +154,43 @@ private Collector createCollector(
String scopedMetricName,
String helpString) {
Collector collector;
- switch (metric.getMetricType()) {
- case GAUGE:
- case COUNTER:
- case METER:
- collector =
- io.prometheus.client.Gauge.build()
- .name(scopedMetricName)
- .help(helpString)
- .labelNames(toArray(dimensionKeys))
- .create();
- break;
- case HISTOGRAM:
- collector =
- new HistogramSummaryProxy(
- (Histogram) metric,
- scopedMetricName,
- helpString,
- dimensionKeys,
- dimensionValues);
- break;
- default:
- log.warn(
- "Cannot create collector for unknown metric type: {}.
This indicates that the metric type is not supported by this reporter.",
- metric.getClass().getName());
- collector = null;
+
+ // Special handling for checkpoint path metric - export as info-style
metric
+ if (scopedMetricName.endsWith(CHECKPOINT_PATH_METRIC_NAME)) {
+ // Add _info suffix to follow Prometheus naming convention for
info metrics
+ String infoMetricName = scopedMetricName + "_info";
+ @SuppressWarnings("unchecked")
Review Comment:
Thanks for your review! In my opinion, the `@SuppressWarnings("unchecked")`
is necessary here because:
1. **Type Safety at Runtime**: The Flink metrics system uses `Gauge<?>` as
the generic type, but we need to cast it to `Gauge<String>` to access the
checkpoint path value. This is a safe cast in this specific context because
we've already verified that this metric is `lastCheckpointExternalPath`, which
is always a string-valued gauge.
2. **Why Not Log**: We don't want to log this as a warning because:
- This is an **expected and intentional** cast, not an error condition
- The metric name check
(`scopedMetricName.endsWith(CHECKPOINT_PATH_METRIC_NAME)`) ensures we only
perform this cast for the correct metric type
- If the cast fails (which shouldn't happen in practice), it will throw a
`ClassCastException` that will be caught and logged by the caller
3. **Alternative Considered**: We could use `instanceof` check, but it would
be redundant since the metric name already uniquely identifies this as a string
gauge.
If you prefer, I can add a comment explaining this, or we could add an
`instanceof` check for extra safety:
```java
if (scopedMetricName.endsWith(CHECKPOINT_PATH_METRIC_NAME)) {
if (metric instanceof Gauge) {
@SuppressWarnings("unchecked")
Gauge<String> pathGauge = (Gauge<String>) metric;
// ... rest of the code
} else {
log.warn("Expected Gauge for checkpoint path metric, but got: {}",
metric.getClass().getName());
return null;
}
}
--
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]
