[GitHub] storm issue #2771: STORM-3157: General improvement to StormMetricsRegistry
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2771 +1, though maybe some of this can be made unnecessary in the future if we decide to make StormMetricsRegistry non-static. Thanks for your patience @zd-project, merged to master. ---
[GitHub] storm issue #2771: STORM-3157: General improvement to StormMetricsRegistry
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2771 Squashed. ---
[GitHub] storm issue #2771: STORM-3157: General improvement to StormMetricsRegistry
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2771 `name()` has been removed from this PR. ---
[GitHub] storm issue #2771: STORM-3157: General improvement to StormMetricsRegistry
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2771 If you don't think the `name()` change makes sense right now, it should probably be removed. If you think there's a reason to keep it, that's fine too. ---
[GitHub] storm issue #2771: STORM-3157: General improvement to StormMetricsRegistry
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2771 change with `name()` is retained for now but have not been incorporated into other PR dependents because I'm not clear if we'll benefit from this. Should I remove this from PR for now and create another PR later if I see the urge to? ---
[GitHub] storm issue #2771: STORM-3157: General improvement to StormMetricsRegistry
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2771 I have rebase this PR onto #2763, and I have also moved out `registerMetricSet`, `unregisterMetricSet` and `registerTimer` to the corresponding patch. ---
[GitHub] storm issue #2771: STORM-3157: General improvement to StormMetricsRegistry
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2771 This will become the base for #2743, #2764, #2754, and #2709. ---
[GitHub] storm issue #2771: STORM-3157: General improvement to StormMetricsRegistry
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2771 As suggested by @Ethanlm, I would like merge #2763 into this one and then port the changes for MetricSet and Timer to the patches where they're actually invoked. How does that sound? @srdo @revans2 ---
[GitHub] storm issue #2771: STORM-3157: General improvement to StormMetricsRegistry
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2771 Still much work here is for better accommodating MetricSet. dropwizard handles MetricSet a bit fuzzy but it's useful for organizing metrics (e.g., this will affect #2764). I'm also starting to seeing values in centralized all metrics for a certain daemon because it's easier to maintain and we would avoid the issue of #2714. I'll soon put up another PR about adding metrics for exceptions in LogViewer, which exemplifies how centralized MetricSet can work better. ---
