[GitHub] flink issue #2112: [FLINK-3951] Add histogram metric type
Github user zentol commented on the issue: https://github.com/apache/flink/pull/2112 merging --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2112: [FLINK-3951] Add histogram metric type
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/2112 I applied the requested changes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2112: [FLINK-3951] Add histogram metric type
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/2112 Thanks for your review @zentol. I've addressed your comments modulo the naming question of `getValue` and the percentile convenience methods. Let's try to figure out what's the best solution for them. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2112: [FLINK-3951] Add histogram metric type
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/2112 Yes the idea is introduce the histogram abstraction which allows us to use any histogram implementation. At the moment we don't have a need for an optimized implementation which might change in the future, though. Thus, for the time being I think it is a good compromise to offer the user the possibility to use Dropwizard histograms in his user code. With that in mind, I don't understand why things are not fitting together? What do you propose? Adding Dropwizard histograms to flink-core? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2112: [FLINK-3951] Add histogram metric type
Github user zentol commented on the issue: https://github.com/apache/flink/pull/2112 So we don't want to tie ourselves to DropWizard histograms, but the only way for users to use a histogram right now _without implementing one themselves (something that is too much work for us apparently)_ is to rely on DropWizard histograms. This doesn't really fit together. _Could_ we add other implementations? Sure! But I have a hard time believing that we'll ever get around to doing that. If it doesn't make sense now to implement one because the DW ones offer so much functionality, when would it? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2112: [FLINK-3951] Add histogram metric type
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/2112 We decided to add the histogram metric, because users requested this feature. The important thing is that the histograms are not used by the runtime per default, because they indeed don't come for free. But if the user decides to use such a metric, then he should be able to explicitly register a histogram. Additionally, we don't want to add a strict dependency on dropwizard from flink-core. Therefore, I introduced the `Histogram` interface which effectively hides the concrete implementation. So later we can easily swap the implementation out. One of these implementations is the `DropwizardHistogramWrapper` which allows you to use Dropwizard histograms in Flink. The reason for this is that Dropwizard's `Histogram` has already a lot of functionality (different reservoirs for sampling streams, for example) and it would be a lot of duplicate work to reimplement it. Since the assumption is that `Histograms` are a user metric, it seems to be ok for me to not ship an implementation of the interface with flink-core but to require the user to include an additional module such as flink-metrics-dropwizard. This module would add the dropwizard dependency anyway. If necessary, then we can also add our own histogram implementation later. The PR does not introduce a Dropwizard compatibility layer. Dropwizard is only one of many possible implementation for our metrics (counter, gauge and histogram so far). It is up to the community to decide which other metrics to add. I agree that the naming of `HistogramWrapper` is not so distinctive. I actually sticked to the naming scheme for `Counter` and `Gauge` which are called `CounterWrapper` and `GaugeWrapper`. This can be easily changed. Do you have a proposal? I'm sorry if you feel angry because you already did some of this work which was later removed. But please keep in mind that not everyone was involved in the metrics PR. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2112: [FLINK-3951] Add histogram metric type
Github user zentol commented on the issue: https://github.com/apache/flink/pull/2112 -1. I haven't looked very deeply at the actual code but let me share some thoughts: I was under the impression that the core of this issue was to implement at least 1 Histogram that does not rely on another library. It _should_ contain at least one to guarantee that the compatibility layer works properly. The original PR for the metric system already contained wrapped DropWIzard Histograms, which were specifically taken out as they did not meet our performance needs. Why we now essentially revert this decision now 2 weeks later is beyond me. I also hope you did not spend too much time writing this as a lot of similar code already existed. As such I really don't see the point of the DropWizardWrapper. IMO the only thing we need is the HistogramWrapper class, which may require a more distinctive name btw. . We should not expose a DW -> Flink compatibility layer, next up people want to use DW Timers, Gauges etc. as well. This will simply create more work for us. The original PR also contained the flink-metric-reporters module as flink-metrics. Now we revert this again as well. Just...wth. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
