You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by eolivelli <gi...@git.apache.org> on 2018/07/24 10:50:44 UTC
[GitHub] zookeeper pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
GitHub user eolivelli opened a pull request:
https://github.com/apache/zookeeper/pull/582
ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper - MetricsProvider API definition
Define the API which must be implemented by a Metrics Provider.
a MetricsProvider is a pluggable component which is able to gather metrics about the system and publish them to an external application for analysis.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/eolivelli/zookeeper fix/metrics
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/582.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #582
----
commit f1c7b778f6177260e26264f4d64c0d7499f8fba9
Author: Enrico Olivelli - Diennea <eo...@...>
Date: 2018-07-24T10:48:30Z
ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper - MetricsProvider API definition
----
---
[GitHub] zookeeper pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r207874562
--- Diff: nbproject/project.xml ---
@@ -0,0 +1,124 @@
+<?xml version="1.0" encoding="UTF-8"?>
--- End diff --
Same here. You don't want to commit your IDE specific files.
---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Posted by lvfangmin <gi...@git.apache.org>.
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 ...
Posted by eolivelli <gi...@git.apache.org>.
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 ...
Posted by eolivelli <gi...@git.apache.org>.
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 pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r205004380
--- Diff: src/java/main/org/apache/zookeeper/metrics/Counter.java ---
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * A counter refers to a value which can only increase.
+ * Usually the value is reset when the process starts.
+ */
+public interface Counter {
+
+ /**
+ * Increment the value by one.
+ */
+ public default void inc() {
+ inc(1);
+ }
+
+ /**
+ * Increment the value by a given amount.
+ *
+ * @param delta
+ */
+ public void inc(long delta);
+
+ /**
+ * Get the current value held by the counter.
+ *
+ * @return the current value
+ */
+ public long getCurrentValue();
--- End diff --
Just change it to get?
---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Posted by hanm <gi...@git.apache.org>.
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 pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r207704611
--- Diff: src/java/main/org/apache/zookeeper/metrics/Gauge.java ---
@@ -0,0 +1,35 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * A Gauge is an application provided object which will be called by the framework in order to sample the value
+ * of an integer value.
+ */
+public interface Gauge {
+
+ /**
+ * Returns the current value associated with this gauge.
+ * The MetricsProvider will call this callback without taking care of synchronization, it is up to the application
+ * to handle thread safety.
+ *
+ * @return the current value for the gauge
+ */
+ long getCurrentValue();
--- End diff --
Okay
---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Posted by eolivelli <gi...@git.apache.org>.
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 ...
Posted by anmolnar <gi...@git.apache.org>.
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 ...
Posted by eolivelli <gi...@git.apache.org>.
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 ...
Posted by anmolnar <gi...@git.apache.org>.
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 pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r207611316
--- Diff: src/java/main/org/apache/zookeeper/metrics/Counter.java ---
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * A counter refers to a value which can only increase.
+ * Usually the value is reset when the process starts.
+ */
+public interface Counter {
+
+ /**
+ * Increment the value by one.
+ */
+ default void inc() {
+ inc(1);
+ }
+
+ /**
+ * Increment the value by a given amount.
+ *
+ * @param delta
+ */
+ void inc(long delta);
+
+ /**
+ * Get the current value held by the counter.
+ *
+ * @return the current value
+ */
+ long get();
--- End diff --
Should we add the similar thread safety comment as Gauge here?
---
[GitHub] zookeeper pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r207705040
--- Diff: src/java/main/org/apache/zookeeper/metrics/Counter.java ---
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * A counter refers to a value which can only increase.
+ * Usually the value is reset when the process starts.
+ */
+public interface Counter {
+
+ /**
+ * Increment the value by one.
+ */
+ default void inc() {
+ inc(1);
+ }
+
+ /**
+ * Increment the value by a given amount.
+ *
+ * @param delta
+ */
+ void inc(long delta);
+
+ /**
+ * Get the current value held by the counter.
+ *
+ * @return the current value
+ */
+ long get();
--- End diff --
Okay
---
[GitHub] zookeeper pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/582
---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Posted by eolivelli <gi...@git.apache.org>.
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 pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r207874376
--- Diff: .gitignore ---
@@ -79,3 +79,4 @@ src/c/depcomp
src/c/install-sh
src/c/ltmain.sh
src/c/missing
+/nbproject/private/
--- End diff --
Please do not commit netbeans related stuff.
---
[GitHub] zookeeper pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r205936167
--- Diff: src/java/main/org/apache/zookeeper/metrics/Counter.java ---
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * A counter refers to a value which can only increase.
+ * Usually the value is reset when the process starts.
+ */
+public interface Counter {
+
+ /**
+ * Increment the value by one.
+ */
+ public default void inc() {
--- End diff --
Sure. Good catch
---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Posted by anmolnar <gi...@git.apache.org>.
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 ...
Posted by anmolnar <gi...@git.apache.org>.
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 ...
Posted by anmolnar <gi...@git.apache.org>.
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 pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r207611064
--- Diff: src/java/main/org/apache/zookeeper/metrics/Gauge.java ---
@@ -0,0 +1,35 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * A Gauge is an application provided object which will be called by the framework in order to sample the value
+ * of an integer value.
+ */
+public interface Gauge {
+
+ /**
+ * Returns the current value associated with this gauge.
+ * The MetricsProvider will call this callback without taking care of synchronization, it is up to the application
+ * to handle thread safety.
+ *
+ * @return the current value for the gauge
+ */
+ long getCurrentValue();
--- End diff --
Maybe change to get() as well.
---
[GitHub] zookeeper pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by pravsingh <gi...@git.apache.org>.
Github user pravsingh commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r205903481
--- Diff: src/java/main/org/apache/zookeeper/metrics/Summary.java ---
@@ -0,0 +1,34 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * Summaries track the size and number of events.
+ * They are able to publish minumum, maximum, average values, depending on the capabilities of the MetricsProvider.
+ */
+public interface Summary {
+
+ /**
+ * Register a value.
+ *
+ * @param value current value
+ */
+ public void registerValue(long value);
--- End diff --
no public.
---
[GitHub] zookeeper pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r207704605
--- Diff: src/java/main/org/apache/zookeeper/metrics/MetricsContext.java ---
@@ -0,0 +1,61 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * Container of metrics.
+ * Contexts are organized in a hierarchy.
+ */
+public interface MetricsContext {
+
+ /**
+ * Returns a sub context.
+ *
+ * @param name the name of the subcontext
+ *
+ * @return a new metrics context.
+ */
+ MetricsContext getContext(String name);
+
+ /**
+ * Returns a counter.
+ *
+ * @param name
+ * @return the counter identified by name in this context.
+ */
+ Counter getCounter(String name);
+
+ /**
+ * Registers an user provided {@link Gauge} which will be called by the MetricsProvider in order to sample
+ * an integer value.
+ *
+ * @param name
+ * @param gauge
+ */
+ void registerGauge(String name, Gauge gauge);
--- End diff --
Yep.
I this this is feasible and it ia usefull
---
[GitHub] zookeeper pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by pravsingh <gi...@git.apache.org>.
Github user pravsingh commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r205902898
--- Diff: src/java/main/org/apache/zookeeper/metrics/Counter.java ---
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * A counter refers to a value which can only increase.
+ * Usually the value is reset when the process starts.
+ */
+public interface Counter {
+
+ /**
+ * Increment the value by one.
+ */
+ public default void inc() {
--- End diff --
public keyword is not needed in interfaces. All of them are public by default
---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Posted by eolivelli <gi...@git.apache.org>.
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 ...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/582
Cool.
Will follow up with the next part.
Thank you
---
[GitHub] zookeeper pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by pravsingh <gi...@git.apache.org>.
Github user pravsingh commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r205903511
--- Diff: src/java/main/org/apache/zookeeper/metrics/MetricsProvider.java ---
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+import java.util.Properties;
+
+/**
+ * A MetricsProvider is a system which collects Metrics and publishes current values to external facilities.
+ *
+ * The system will create an instance of the configured class using the default constructor, which must be public.<br>
+ * After the instantiation of the provider, the system will call {@link #configure(java.util.Map) } in order to provide configuration,
+ * and then when the system is ready to work it will call {@link #start() }.
+ * <br>
+ * Providers can be used both on ZooKeeper servers and on ZooKeeper clients.
+ */
+public interface MetricsProvider {
+
+ /**
+ * Configure the provider.
+ *
+ * @param configuration the configuration.
+ *
+ * @throws MetricsProviderLifeCycleException in case of invalid configuration.
+ */
+ public void configure(Properties configuration) throws MetricsProviderLifeCycleException;
--- End diff --
no public
---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Posted by eolivelli <gi...@git.apache.org>.
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 pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r205047955
--- Diff: src/java/main/org/apache/zookeeper/metrics/Counter.java ---
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * A counter refers to a value which can only increase.
+ * Usually the value is reset when the process starts.
+ */
+public interface Counter {
+
+ /**
+ * Increment the value by one.
+ */
+ public default void inc() {
+ inc(1);
+ }
+
+ /**
+ * Increment the value by a given amount.
+ *
+ * @param delta
+ */
+ public void inc(long delta);
+
+ /**
+ * Get the current value held by the counter.
+ *
+ * @return the current value
+ */
+ public long getCurrentValue();
--- End diff --
@lvfangmin done (I did a squash commit, GitHub lost the binding to the code)
---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Posted by eolivelli <gi...@git.apache.org>.
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 ...
Posted by eolivelli <gi...@git.apache.org>.
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 pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r207611617
--- Diff: src/java/main/org/apache/zookeeper/metrics/MetricsContext.java ---
@@ -0,0 +1,61 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * Container of metrics.
+ * Contexts are organized in a hierarchy.
+ */
+public interface MetricsContext {
+
+ /**
+ * Returns a sub context.
+ *
+ * @param name the name of the subcontext
+ *
+ * @return a new metrics context.
+ */
+ MetricsContext getContext(String name);
+
+ /**
+ * Returns a counter.
+ *
+ * @param name
+ * @return the counter identified by name in this context.
+ */
+ Counter getCounter(String name);
+
+ /**
+ * Registers an user provided {@link Gauge} which will be called by the MetricsProvider in order to sample
+ * an integer value.
+ *
+ * @param name
+ * @param gauge
+ */
+ void registerGauge(String name, Gauge gauge);
--- End diff --
Maybe return boolean to indicate if we successfully registered the gauge or not. For example, adding an already exist Gauge should return false.
---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Posted by lvfangmin <gi...@git.apache.org>.
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 pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r205008065
--- Diff: src/java/main/org/apache/zookeeper/metrics/Counter.java ---
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * A counter refers to a value which can only increase.
+ * Usually the value is reset when the process starts.
+ */
+public interface Counter {
+
+ /**
+ * Increment the value by one.
+ */
+ public default void inc() {
+ inc(1);
+ }
+
+ /**
+ * Increment the value by a given amount.
+ *
+ * @param delta
+ */
+ public void inc(long delta);
+
+ /**
+ * Get the current value held by the counter.
+ *
+ * @return the current value
+ */
+ public long getCurrentValue();
--- End diff --
okay, will do
---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Posted by eolivelli <gi...@git.apache.org>.
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 ...
Posted by eolivelli <gi...@git.apache.org>.
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 pull request #582: ZOOKEEPER-3103 Pluggable metrics system for Zoo...
Posted by pravsingh <gi...@git.apache.org>.
Github user pravsingh commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/582#discussion_r205902926
--- Diff: src/java/main/org/apache/zookeeper/metrics/Gauge.java ---
@@ -0,0 +1,35 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.metrics;
+
+/**
+ * A Gauge is an application provided object which will be called by the framework in order to sample the value
+ * of an integer value.
+ */
+public interface Gauge {
+
+ /**
+ * Returns the current value associated with this gauge.
+ * The MetricsProvider will call this callback without taking care of synchronization, it is up to the application
+ * to handle thread safety.
+ *
+ * @return the current value for the gauge
+ */
+ public long getCurrentValue();
--- End diff --
no public.
---
[GitHub] zookeeper issue #582: ZOOKEEPER-3103 Pluggable metrics system for ZooKeeper ...
Posted by eolivelli <gi...@git.apache.org>.
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
---