[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2783 Thanks ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2783 I'm really sorry about the timing. I'll try to take a look at it once the merge is done. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2783 I'll close this in the meantime. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2783 @zd-project Alright, maybe the best option is that we postpone looking at this until the added metrics have been merged. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2783 I can port changes on top of this and open another PR, but I hope existing PRs can be merged in first, as they provide actual metrics that are straight useful to system administrators and developers. While this is more of a improvement to internals. Additionally, my internship ends this week and I will go back to school by the end of August, so though best I try, I can't guarantee to contribute regularly thereafter. We should plan this accordingly. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2783 Rebased to master now that the UI work has been merged. @zd-project How would you like to proceed? If you feel that making this change is risky, we could maybe try to port some of your PRs that add new metrics to be based on this branch, to see if having the registry be non-static adds a lot of difficulty? ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2783 I think it depends a lot on what kind of static method we're talking about. If the method is internal we can just change it and inject the metric, or even better make the method not static and inject the metrics or other dependencies via the constructor. This is sort of what happened with the memory tracking in Container. I see your point for publically accessible static methods though. I'm not sure we can do anything to work around it there. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2783 Additionally, it still seems to be not ideal that if we want to measure things inside a static method, we'd have to pass the metric in as a parameter. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2783 @zd-project You are right that if someone wants to add metrics to a component, they will have to figure out how to get the registry injected. I can't say up front how hard that will be, except to note that it wasn't too bad to make these changes. For the specific case of the Container, I put the metrics in the ContainerMemoryTracker next to the fields they're gauges for. This is primarily because Container gets created a lot of times, and I didn't want to call the `gauge` method every time the constructor is invoked. In this case I chose to move the gauges to a different class with a different lifecycle from Container, but since the `gauge` method uses `getOrAdd` there probably wouldn't be anything wrong with registering the metrics every time a Container is created instead. If I were adding new metrics to Container, I would either make a metrics container object like SlotMetrics that is common to all Containers and pass that in, or pass in the registry and just invoke the registration every time the constructor is invoked. Unless there's a big performance penalty to using `getOrAdd`, I would think that there wouldn't be much difference. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2783 Extensibility and Centralized management would be my concerns. I think this PR has improved on the centralized management of metrics. For example I think the slotMetrics class is a great example of this (it's actually a very similar idea to the MetricSet I've put in). It's easier to tell this way what we have been tracking for a certain daemon and how we're tracking it. But I'm not quite sure about the extensibility part, i.e., what I should do if I want to have metrics in a new component or one that hasn't been injected a registry yet. For example, in my PR I added a few metrics in Container, but I don't think I see a way to add them here since Container doesn't have access to StormMetricsRegistry. We might need to do some refactoring before we can actually add in metrics. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2783 I love all the approaches to switch static to non-static if it doesn't require any hack or long parameters to inject, and this looks great. +1 to move on. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2783 The test failure is unrelated, caused by https://issues.apache.org/jira/browse/STORM-3166 ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user agresch commented on the issue: https://github.com/apache/storm/pull/2783 I like this. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2783 Just to make clear, this shouldn't be merged. If we decide to solve the unintended registration issue like this, I'll make a PR into https://github.com/apache/storm/pull/2714. ---
