-
Notifications
You must be signed in to change notification settings - Fork 598
Export heron-instances' streams-aggregated metrics #2020
base: master
Are you sure you want to change the base?
Conversation
… streams. But a lot of customers expressed their interest on the value of all streams aggregated and for now they have to sum them up by themselves. This pull request exports metrics aggregated on differetn streams too. Tested with LocalScheduler.
public interface ComponentMetrics { | ||
public abstract class ComponentMetrics { | ||
// Metric-name suffix reserved for value aggregating on all different streams | ||
public static final String ALL_STREAMS_AGGREGATED = "__all-streams-aggregated"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we put this constant string into a separate final class like MetricsConstants
?
Making a variable public in interface or abstract class smells dangerous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One the one hand, it seems we don't have such Constants class; I don't see how necessary to add a class for a single filed;
on the other hand, this class is designed to abstract out the shared actions/implementations, which can be a good place to put a shared field.
BTW, I would prefer this class being an abstract class to an interface, since IMO this class is more used to reduce duplicated code, rather than to define some required behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making a field public while allowing the class to be extended is in general a bad design.
For the problem why it's an interface, @billonahill may have a better idea.
To land this metric fast, I'll approve it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No rush for this pull request. Will also wait for Bill's comments.
@maosongfu Could you please elaborate on how the customers use the aggregated metrics. Also graphite and other tools provide easy mechanism to aggregate metrics. Do we really need to worry about the intent inside the code? In my opinion metrics aggregation is better handled by tools. |
@ashvina Sure. It is desired to handle by the tools. But:
|
Thanks @maosongfu |
@ashvina From overhead point of view, doing aggregation on heron-instance side will be cheapest , most scalable and easy to implement: it will be an in-memory java hashmap get() and put(); also the aggregation will be done distributedly in different heron-instances.
Currently the common practice is that metrics-sinks in metricsmgr can handle the metrics in their own ways. For instance, scribe-sink/graphite-sink will send those metrics to Scribe/Graphite directly. It is unlikely to perform the aggregation at topology level. |
Currently heron-instances exports most of metrics scoped by different streams.
But a lot of customers expressed their interest on the value of all streams aggregated and for now they have to sum them up by themselves.
This pull request exports metrics aggregated on different streams too.
Tested with LocalScheduler.