ZOOKEEPER-5036: Add Timeline Metrics Provider for external metrics collection systems#2371
ZOOKEEPER-5036: Add Timeline Metrics Provider for external metrics collection systems#2371himanshumaurya09876 wants to merge 1 commit intoapache:masterfrom
Conversation
himanshumaurya09876
force-pushed
the
zookeeper-5036
branch
from
037f537 to
1778c05
Compare
|
/build |
There was a problem hiding this comment.
Many thanks, looks really nice already. 👍
I really like the clean separation of concerns: Provider/Sink/Snapshot are well-decomposed. 💪
Can you please add unit tests for the new classes?
Also, in order to be able to load the metrics provider when running from the source clone, we have to add it to the classpath. See:
| <parent> | ||
| <groupId>org.apache.zookeeper</groupId> | ||
| <artifactId>zookeeper-metrics-providers</artifactId> | ||
| <version>3.9.3</version> |
There was a problem hiding this comment.
Shouldn't this match with other projects to refer to 3.10.0-SNAPSHOT?
| <version>3.9.3</version> | |
| <version>3.10.0-SNAPSHOT</version> |
This also seems to cause Maven compilation problem.
| /** | ||
| * Returns all counter metrics in this snapshot. | ||
| * | ||
| * @return an unmodifiable view of the counters map |
There was a problem hiding this comment.
This claims to return "an unmodifiable view" but return the raw HashMap directly. Should be wrapped with Collections.unmodifiableMap() or remove this claim here?
The same applies to getGauges(), getSummaries().
| * <p>This context reuses existing metric implementations from zookeeper-server | ||
| * (SimpleCounter, AvgMinMaxCounter, etc.) to ensure consistent behavior with | ||
| * other metrics providers.</p> |
There was a problem hiding this comment.
I like that you reused the existing metric classes (SimpleCounter, AvgMinMaxCounter, etc.) 👍
| LOG.warn("Timeline sink class not found: {}. Timeline metrics will be disabled. " | ||
| + "To enable Timeline metrics, ensure the sink implementation JAR is available on the classpath. " | ||
| + "ZooKeeper will continue to operate normally without Timeline metrics.", sinkClassName); | ||
| this.sink = null; |
There was a problem hiding this comment.
As I see when the sink class isn't found, the provider logs a warning but continues. It can be like that but PrometheusMetricsProvider throws an Exception when misconfigured and ZooKeeper will fail to start. Would it maybe make sense to be consistent in behavior? What do you all think?
2 participants
Added a new Timeline Metrics Provider that enables ZooKeeper to export metrics to external Timeline-based metrics collection systems such as Apache Ambari Metrics Collector and YARN Timeline Service.
Validated it using Ambari Metrics, I was able to receive Zookeeper Metrics in Ambari and visualise it on Grafana dashboards.
Jira :- ZOOKEEPER-5036