search

salesforce / storm-dynamic-spout

A framework for building spouts for Apache Storm and a Kafka based spout for dynamically skipping messages to be processed later.
BSD 3-Clause "New" or "Revised" License
41 stars 13 forks source link

Upgrade to Storm 1.2 metrics methods #117

Closed stanlemon closed 6 years ago

stanlemon commented 6 years ago

Storm 1.1 and lower offered a method on TopologyContext called registerMetrics() (see https://github.com/apache/storm/blob/1.1.x-branch/storm-core/src/jvm/org/apache/storm/task/TopologyContext.java). In Storm 1.2 these methods were deprecated and registerTimer(), registerHistogram(), registerMeter(), registerCounter() and registerGauge() were added (see https://github.com/apache/storm/blob/v1.2.2/storm-core/src/jvm/org/apache/storm/task/TopologyContext.java#L396-L418). Within this library we represent several metric types via the MetricsRecorder interface, specifically countBy(), assignValue() and recordTimer() along with a set of helpers for each. Our current metrics methods map to three of these, but we have gaps for meters (measures mean throughput and one-, five-, and fifteen-minute exponentially-weighted moving average throughputs) and histograms (calculates the distribution of a value).

In the short term (0.10) we can swap StormRecorder (the implementation of MetricsRecorder that leverages TopologyContext to call registerMetrics() to use these new methods. This does then impose a hard requirement on Storm 1.2 because the new metrics methods did not exist before that release. Currently 0.10 requires 1.2, but it should function fine with older version - this would not be the case with a metrics change. Longer term do we need to consider exposing the other two metrics methods? As it stands today we don't have a use case currently, but we do pass the MetricsRecorder instance downstream to many classes that can be overridden through third party extension.

stanlemon commented 6 years ago

Offline discussion has me leaning toward a second metrics implementation and keeping the deprecated one around in the interim. This won't resolve the deprecation notices though. 😿 I will probably mark StormRecorder as @Deprecated then too.

stanlemon commented 6 years ago

This is resolved with #119.