Improvements to MetricsSnapshotReporter #518
Conversation
…pshot reporter. Switched producer-shutdown logic to ensure metric-flush at shutdown-time
rayman7718
changed the title
vjagadish1989
left a comment
vjagadish1989
left a comment
There was a problem hiding this comment.
Initial set of comments. Lets get a quick review from @prateekm as well since he has familiarity with the metrics system.
| * All public methods are thread-safe. | ||
| * | ||
| */ | ||
| public class ListGauge implements Metric { |
There was a problem hiding this comment.
What do you think about this as a list of values? You don't have to expose the fact that it uses a Gauge underneath. ListGauge merely takes in an add(T val)
| * Add a gauge to the list | ||
| * @param value The Gauge value to be added | ||
| */ | ||
| public synchronized void add(Gauge value) { |
There was a problem hiding this comment.
Can just take in a T value
| * All public methods are thread-safe. | ||
| * | ||
| */ | ||
| public class ListGauge implements Metric { |
There was a problem hiding this comment.
Would be nice to parameterize this class? ListGauge<T> since Gauges are parameterized already
| */ | ||
| public synchronized void add(Gauge value) { | ||
| if (this.metricList.size() == nItems) { | ||
| ((LinkedList<Gauge>) this.metricList).removeFirst(); |
There was a problem hiding this comment.
You probably don't need this cast here?
| * @return the collection of gauge values | ||
| */ | ||
| public synchronized Collection<Gauge> getValue() { | ||
| return this.metricList; |
There was a problem hiding this comment.
Should we return a copy or wrap it in an immutable collection so that modifications don't alter the original?
|
|
||
| producer.stop | ||
| // Scheduling an event with 0 delay to ensure flushing of metrics one last time before shutdown | ||
| executor.schedule(this,0, TimeUnit.SECONDS) |
There was a problem hiding this comment.
nit pick: spacing in commas
| import java.util.LinkedList; | ||
|
|
||
|
|
||
| /** |
There was a problem hiding this comment.
Addressed these comments in new, separate PR: #541
| * | ||
| * This current implementation uses a size-bound-policy and holds the N most-recent Gauge objects added to the list. | ||
| * This bound N is configurable at instantiation time. | ||
| * TODO: Support a time-based and size-and-time-based hybrid policy. |
There was a problem hiding this comment.
If this PR is only adding gauges, maybe move the TODOs to another JIRA and assign them to yourself. You can remove these from code.
| * This current implementation uses a size-bound-policy and holds the N most-recent Gauge objects added to the list. | ||
| * This bound N is configurable at instantiation time. | ||
| * TODO: Support a time-based and size-and-time-based hybrid policy. | ||
| * TODO: Add a derived class to do compaction for errors using hash-based errorIDs and adding timestamp for errors to dedup |
There was a problem hiding this comment.
Maybe this should be a new metric type then? would be good to think what the end-user API is so that we build the right thing.
|
also, what do you think about editing this PR title to something on the lines of: "Add a ListGauge metric type". IMO, It's more indicative of what its contents are. |
… to hostname (to maintain backword compat)
|
As discussed offline, created a separate pull requests for |
3 participants
…pshot reporter.
Switched producer-shutdown logic to ensure metric-flush at shutdown-time