[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 retest this please ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/582 thanks for quick work @eolivelli . I'll have a more close look on this tomorrow. Just quick comment - I am not sure if you can kick off jenkins by just add a comment. You can kick jenkins bot either by pushing a new commit, or close/repoen the pull request. ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user lvfangmin commented on the issue: https://github.com/apache/zookeeper/pull/582 One question is are we just publishing the raw data to the external metric report system, or we need to do our aggregation to find out values like min/avg/max? If we need to aggregate before report, it might be useful to take a look at ZOOKEEPER-3098, which added AvgMinMaxCounter, SimpleCounter. We also added percentile counter, etc, which will be contributed later. ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 @lvfangmin > One question is are we just publishing the raw data to the external metric report system, or we need to do our aggregation to find out values like min/avg/max? > > If we need to aggregate before report, it might be useful to take a look at ZOOKEEPER-3098, which added AvgMinMaxCounter, SimpleCounter. We also added percentile counter, etc, which will be contributed later. I am aware of ZOOKEEPER-3098. In my idea it is up to the MetricsProvider to export percentiles, averages, min, max This is what usually Prometheus and Dropwizard Metrics do automatically, you only have to provide raw data and all aggregations are computed internally. This is very efficient and makes the application able to leverage internal provider optimizations see just as an example: https://github.com/prometheus/client_java#summary we can provide our own "simple" ZookKeeper Basic Metrics Provider which uses your Metrics utilities in ZOOKEEPER-3098, it will be only a refactor of existing code. Beside topic: I am thinking about an integration with the four letter words endpoint: In BookKeeper we have a bridge from Metrics Providers and HTTP Admin API which enables every provider to dump its state in a common text based format, we can follow a similar approach for ZK see: https://github.com/apache/bookkeeper/blob/16553057b0ddba53ac169c4fef81336e2bd26116/bookkeeper-stats/src/main/java/org/apache/bookkeeper/stats/StatsProvider.java#L45 ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 @pravsingh thank you for taking a look. We are going to design a zookeeper proprierary internal API mostly because we want the project to be owner of its own internal instrumentation system. It is more like defining a standard way to instrument zookeeper code. See other comments in JIRA and on this PR. We will surely provide some out of the box integration but we want zookeeper to have as few as possible dependencies to maintain in the future ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 @hanm @anmolnar do you have cycles for review? If this approach is good I can send the second part, about booting the provider in the server and providing some sample instrumentation ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/582 @eolivelli I'll take a look today. ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 @anmolnar that you for catching those file, I did a 'git add .' too aggressively. @lvfangmin I have addressed you comments ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user lvfangmin commented on the issue: https://github.com/apache/zookeeper/pull/582 +1 Thanks @eolivelli. The new change looks good to me. ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 Shall we commit this patch ? So we can make little steps forward. This change does not impact how zk works, it is safe ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/582 Sorry @eolivelli I really don't want to block this patch, but I'd like to get a better understanding on the roles of the classes/interfaces here. - What's the original concept that you're replicating here if there's any? Bookeeper/Hadoop? They would be good examples of where we're heading. - What's the role of a MetricsProvider/MetricsContext/MetricsProviderLifeCycleException? Unfortunately the javadocs don't explain them in detail. Thanks. ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 @anmolnar thank you for following up on this. Background of this issue is on JIRA https://issues.apache.org/jira/browse/ZOOKEEPER-3092 the plan is the following: - send this patch and discuss the API for a MetricsProvider - send a second patch for the boostrap of the MetricsProvider + NOOP implementation + minimal instrumentation (as an example) - send the patch with full instrumentation of all the current metrics exposed in 4LW - start the same work on the client Examples are: - Prometheus - Dropwizard - BookKeeper Stats Loggers API - Hadoop Metrics I can send the second patch if it is worth but I would like to discussion to be as narrow as possible, a, huge patch won't be reviewable. At the moment this is the high level API, we will instrument all the code with probes, starting from all of the current instrumented points. I think sharing a Google Doc won't help so much, it is better to share code in this particular case ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/582 Thanks @eolivelli, I got that. However, what I'm interested in is slightly different. :) Let me try to rephrase it: I'd like to have an idea of what the different interfaces that you're introducing in this patch are going to be used for. For example: - `MetricsProvider` If I understand it correctly, this is going to be the wrapper of different metrics providers like Prometheus, Dropwizard, etc. - `Counter`/`Gauge`/`Summary` Interfaces for the most commonly used generic metrics behaviours. - `MetricsContext` ?? (I have no idea what it is) - `MetricsProviderLifeCycleException` Parent exception of all lifecycle exceptions of a metrics provider. After all - if my above summary is pretty much correct - the only interface which needs some clarification is the `MetricsContext`. Also it would be useful to add more context to its javadoc with some useful examples potentially. Sorry if you already explained it somewhere else and I just can't find it myself. ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 @anmolnar you are right. MetricsContext will be like a namespace, I expect that each component/submodule will have its own MetricsContext. This is useful for two reasons/usecases. 1) each component will work having its own MetricsContext, which is like a local namespace for metrics 2) there are cases in which you have multiple instances of each component, for instance in a zk server maybe you will have the same metrics for every other peer Maybe it is worth to add some more doc, but when we will have attached the interface to real code it will be very straightforward. ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 @anmolnar you are right. MetricsContext will be like a namespace, I expect that each component/submodule will have its own MetricsContext. This is useful for two reasons/usecases. 1) each component will work having its own MetricsContext, which is like a local namespace for metrics 2) there are cases in which you have multiple instances of each component, for instance in a zk server maybe you will have the same metrics for every other peer Maybe it is worth to add some more doc, but when we will have attached the interface to real code it will be very straightforward. ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 I will add more docs. Seems that we are agreeing on this way. I will send a new patch based on this branch (or on master if we merge this patch) New step is about booting a MetricsProvider and a dummy noop implementation ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/582 Thanks @eolivelli . It makes perfect sense to me now. Adding more docs (especially javadoc in this case) is always a good idea. Copying the above description to javadoc is sufficient I believe. I'm happy to commit once you're ready and have a green build. ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 @anmolnar I have updated the docs. Using ant javadoc will not produce an usefull javadoc site, most package will be empty. Which is the command to use to build the full docs ? ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 Btw I think this is good to go now ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/582 Committed to master with 2 approvals. Thanks @eolivelli ---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/582 Cool. Will follow up with the next part. Thank you ---
