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/27 12:10:51 UTC
[GitHub] [flink] qingwei91 opened a new pull request, #21403: [FLINK-29984][flink-metrics] Prometheus histogram minmax
qingwei91 opened a new pull request, #21403:
URL: https://github.com/apache/flink/pull/21403
## What is the purpose of the change
Expose Histogram min and max when exporting to Prometheus, the data is already available, we just need to export it.
This is enabled in both Prometheus and Prometheus Gateway reporter.
## Verifying this change
This change added tests and can be verified as follows:
* Added test to make sure min and max are emitted in prometheus reporter.
## Does this pull request potentially affect one of the following parts
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
- The S3 file system connector: (no)
## Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (docs)
--
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
[GitHub] [flink] qingwei91 commented on pull request #21403: [FLINK-29984][flink-metrics] Prometheus histogram minmax
Posted by GitBox <gi...@apache.org>.
qingwei91 commented on PR #21403:
URL: https://github.com/apache/flink/pull/21403#issuecomment-1355422224
Hi @xintongsong , sorry was quite busy in the past 2 weeks, will try to continue this week. Hope that's ok
--
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
[GitHub] [flink] xintongsong commented on a diff in pull request #21403: [FLINK-29984][flink-metrics] Prometheus histogram minmax
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [flink] flinkbot commented on pull request #21403: [FLINK-29984][flink-metrics] Prometheus histogram minmax
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #21403:
URL: https://github.com/apache/flink/pull/21403#issuecomment-1328234175
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "d916abd22d6bb510b39df1c51b04f74ad92c8d59",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "d916abd22d6bb510b39df1c51b04f74ad92c8d59",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* d916abd22d6bb510b39df1c51b04f74ad92c8d59 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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
[GitHub] [flink] xintongsong commented on pull request #21403: [FLINK-29984][flink-metrics] Prometheus histogram minmax
Posted by GitBox <gi...@apache.org>.
xintongsong commented on PR #21403:
URL: https://github.com/apache/flink/pull/21403#issuecomment-1352740095
@qingwei91, any updates on this?
--
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
[GitHub] [flink] qingwei91 commented on pull request #21403: [FLINK-29984][flink-metrics] Prometheus histogram minmax
Posted by GitBox <gi...@apache.org>.
qingwei91 commented on PR #21403:
URL: https://github.com/apache/flink/pull/21403#issuecomment-1356473916
I will reopen once its ready
--
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
[GitHub] [flink] qingwei91 closed pull request #21403: [FLINK-29984][flink-metrics] Prometheus histogram minmax
Posted by GitBox <gi...@apache.org>.
qingwei91 closed pull request #21403: [FLINK-29984][flink-metrics] Prometheus histogram minmax
URL: https://github.com/apache/flink/pull/21403
--
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
[GitHub] [flink] qingwei91 commented on a diff in pull request #21403: [FLINK-29984][flink-metrics] Prometheus histogram minmax
Posted by GitBox <gi...@apache.org>.
qingwei91 commented on code in PR #21403:
URL: https://github.com/apache/flink/pull/21403#discussion_r1032920245
##########
flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/AbstractPrometheusReporter.java:
##########
@@ -378,6 +390,22 @@ private void addSamples(
addToList(labelValues, quantile.toString()),
statistics.getQuantile(quantile)));
}
+ if (this.histogramMaxEnabled) {
+ samples.add(
+ new MetricFamilySamples.Sample(
+ metricName,
+ labelNamesWithQuantile,
+ addToList(labelValues, "1.0"),
+ statistics.getMax()));
+ }
+ if (this.histogramMinEnabled) {
+ samples.add(
+ new MetricFamilySamples.Sample(
+ metricName,
+ labelNamesWithQuantile,
+ addToList(labelValues, "0.0"),
+ statistics.getMin()));
+ }
Review Comment:
Core of the changes, emit max as 1.0 and min as 0.0 when reporting to prometheus
--
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