xintongsong commented on code in PR #21403:
URL: https://github.com/apache/flink/pull/21403#discussion_r1033312994
##########
flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusPushGatewayReporterOptions.java:
##########
@@ -111,8 +111,18 @@ public class PrometheusPushGatewayReporterOptions {
"Prometheus requirements"))
.build());
public static final ConfigOption<Boolean> ENABLE_HISTOGRAM_MAX =
- PrometheusReporterOptions.ENABLE_HISTOGRAM_MAX;
+ ConfigOptions.key("histogramMax")
Review Comment:
These are not documentation changes, as described by the commit message.
##########
flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusReporterOptions.java:
##########
@@ -0,0 +1,26 @@
+package org.apache.flink.metrics.prometheus;
+
+import org.apache.flink.annotation.docs.Documentation;
+import org.apache.flink.configuration.ConfigConstants;
+import org.apache.flink.configuration.ConfigOption;
+import org.apache.flink.configuration.ConfigOptions;
+
+/** Config options of PrometheusReporter. */
[email protected](ConfigConstants.METRICS_REPORTER_PREFIX +
"prometheus")
+public class PrometheusReporterOptions {
Review Comment:
Why do we need to introduce this class and then reference it from
`PrometheusPushGatewayReporterOptions.java`?
##########
flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusReporterOptions.java:
##########
@@ -1,26 +0,0 @@
-package org.apache.flink.metrics.prometheus;
-
-import org.apache.flink.annotation.docs.Documentation;
-import org.apache.flink.configuration.ConfigConstants;
-import org.apache.flink.configuration.ConfigOption;
-import org.apache.flink.configuration.ConfigOptions;
-
-/** Config options of PrometheusReporter. */
[email protected](ConfigConstants.METRICS_REPORTER_PREFIX +
"prometheus")
-public class PrometheusReporterOptions {
- public static final ConfigOption<Boolean> ENABLE_HISTOGRAM_MAX =
- ConfigOptions.key("enable_histogram_max")
- .booleanType()
- .defaultValue(true)
- .withDescription(
- "Specifies whether to enable maximum from
histogram or not. If enabled, "
- + "the histogram metrics will include
quantile=1.0 which is equivalent to max.");
-
- public static final ConfigOption<Boolean> ENABLE_HISTOGRAM_MIN =
- ConfigOptions.key("enable_histogram_min")
- .booleanType()
- .defaultValue(true)
- .withDescription(
- "Specifies whether to enable minimum from
histogram or not. If enabled, "
- + "the histogram metrics will include
quantile=0.0 which is equivalent to min.");
-}
Review Comment:
Commits in a PR should be self-contained. In the same PR, there shouldn't be
things like making something wrong in one commit and fix it in another later
commit.
##########
docs/layouts/shortcodes/generated/prometheus_push_gateway_reporter_configuration.html:
##########
@@ -26,6 +26,18 @@
<td>String</td>
<td>Specifies the grouping key which is the group and global
labels of all metrics. The label name and value are separated by '=', and
labels are separated by ';', e.g., <code
class="highlighter-rouge">k1=v1;k2=v2</code>. Please ensure that your grouping
key meets the <a
href="https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels";>Prometheus
requirements</a>.</td>
</tr>
+ <tr>
+ <td><h5>metrics.reporter.prometheus.histogramMax</h5></td>
+ <td style="word-wrap: break-word;">true</td>
+ <td>Boolean</td>
+ <td>Specifies whether to enable maximum from histogram or not. If
enabled, the histogram metrics will include quantile=1.0 which is equivalent to
max.</td>
+ </tr>
+ <tr>
+ <td><h5>metrics.reporter.prometheus.histogramMin</h5></td>
+ <td style="word-wrap: break-word;">true</td>
+ <td>Boolean</td>
+ <td>Specifies whether to enable minimum from histogram or not. If
enabled, the histogram metrics will include quantile=0.0 which is equivalent to
min.</td>
+ </tr>
Review Comment:
HTML documentations should be generated in the same commit that the
configuration options are changed.
##########
flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/AbstractPrometheusReporter.java:
##########
@@ -321,16 +326,23 @@ static class HistogramSummaryProxy extends Collector {
private final Map<List<String>, Histogram> histogramsByLabelValues =
new HashMap<>();
+ private final Boolean histogramMaxEnabled;
+ private final Boolean histogramMinEnabled;
Review Comment:
I think we should make the entire `QUANTILES` a configurable list, rather
than introducing min/max as two special cases.
--
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]
