You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/11/28 09:41:08 UTC
[GitHub] [flink] xintongsong commented on a diff in pull request #21403: [FLINK-29984][flink-metrics] Prometheus histogram minmax
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. */
+@Documentation.SuffixOption(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. */
-@Documentation.SuffixOption(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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org