Flink: Do not ship optional flink-metrics-dropwizard dependency#16155
Flink: Do not ship optional flink-metrics-dropwizard dependency#16155mxm wants to merge 17 commits intoapache:mainfrom
Conversation
Flink does not bundle flink-metrics-dropwizard, which enables histogram metrics. Users need to manually include the dependency for histograms to work. In Iceberg, we used to include the dependency in the flink-runtime jar. Ryan had removed the dependency in apache#16093 to avoid LICENSE issues, but we added it back in apache#16126 with the corresponding LICENSE changes, to avoid breaking changes for users. After discussing with Peter, we decided to remove the bundling going forward. This PR makes sure there are no regressions removing the dependency. I've updated the docs and the changelog to inform users of the change. References: - https://nightlies.apache.org/flink/flink-docs-stable/docs/ops/metrics/#histogram - https://github.com/apache/flink/blob/ccf45727eeb2e920f7939f453db67a411e3cb109/flink-dist/pom.xml#L284
Do you mean after 1.11.0 release? |
For 1.11.0 already, unless there are any objections raised in this PR. I updated the description to state that. |
| </dependencies> | ||
| ``` | ||
|
|
||
| ### 1.11.0 release |
There was a problem hiding this comment.
This will be published immediately while 1.11.0 is not released yet.
There was a problem hiding this comment.
Where do we put the pending release notes?
There was a problem hiding this comment.
We don't have such a place. Maybe open an issue for @aihuaxu to track?
There was a problem hiding this comment.
I've reverted the change and created #16170. Unfortunately, I couldn't tag it with the release tag.
I wonder whether it would make sense to create a file in the repo with the pending release notes? Seems easier to maintain.
| this.dataFilesSizeHistogram = | ||
| writerMetrics.histogram( | ||
| "dataFilesSizeHistogram", | ||
| new DropwizardHistogramWrapper(dropwizardDataFilesSizeHistogram)); |
There was a problem hiding this comment.
I might miss something - just wonder why Flink's built-in DescriptiveStatisticsHistogram does not work here?
There was a problem hiding this comment.
@stevenzwu: Do you happen to remember why we chose DropwizardHistogramWrapper originally?
There was a problem hiding this comment.
According to the Flink docs, using Dropwizard metrics is still the official way to use Histogram metrics. DescriptiveStatisticsHistogram was added to flink-runtime for internal metrics around the same time of the initial implementation here. The semantics (e.g. bucketing) of the internal histogram are different. I would defer this to a follow-up, as the behavior of the implementation might change.
|
|
||
| private static Histogram registerHistogram(MetricGroup group, String name) { | ||
| Histogram histogram = newDropwizardHistogram(); | ||
| return histogram != null ? group.histogram(name, histogram) : null; |
There was a problem hiding this comment.
Is group.histogram just the histogram that is passed to it? If so then you may not need this method. It looks good either way though.
There was a problem hiding this comment.
Yes, group.histogram registers the passed histogram and then returns it. I'd prefer to keep the method, because inlining would make the histogram initialization more verbose.
|
I prefer keeping the CTORS wrapped in a static field. The PR is ready from my side. Thank you for your thoughtful comments @pvary @rdblue @manuzhang @pan3793! |
5 participants
Flink does not bundle
flink-metrics-dropwizard[1][2], which enables histogram metrics. Users need to manually include the dependency for histograms to work.In Iceberg, we used to include the dependency in the flink-runtime jar. Ryan had removed the dependency in #16093 to avoid LICENSE issues, but we added it back in #16126 with the corresponding LICENSE changes, to avoid breaking changes for users.
After discussing with Peter, we decided to remove the bundling going forward (
1.11.0and later). This PR makes sure there are no regressions removing the dependency. I've updated the docs and the changelog to inform users of the change.References: