Append the Storm component's task ID to the metric name, too #1
Conversation
This is a workaround on Storm's imprecise metric emit interval, which oscillates wildly around the user-defined metric emit interval value. As Storm could (and does) emit multiple metrics in a single defined time window, Statsd counts would be aggregated by Statsd (and of course, duplicated), but gauges don't aggregate, so they're a safe alternative. It would be best to make the Statsd metric type emitted user-configurable.
| @@ -139,7 +139,8 @@ public String toString() { | |||
| StringBuilder sb = new StringBuilder() | |||
| .append(clean(taskInfo.srcWorkerHost)).append(".") | |||
| .append(taskInfo.srcWorkerPort).append(".") | |||
| .append(clean(taskInfo.srcComponentId)).append("."); | |||
| .append(clean(taskInfo.srcComponentId)).append(".") | |||
| .append(taskInfo.srcTaskId).append("."); | |||
There was a problem hiding this comment.
I originally did not include the taskID on purpose. It is because the task ID assignment seems arbitrary (i.e. the same taskID is not assigned to the same worker/port) across topology restarts. So, to view this data in any sensible view in graphite, you would have to use sumSeries() and a wildcard on the taskID. To simplify things I just didn't include it.
I would probably be more open to making this a configurable option (e.g. metrics.statsd.includeTaskID) so it can be turned on/off, with the default being off.
There was a problem hiding this comment.
+1 It would be great to make the task ID configurable! Especially since appending the task ID is what the the default LoggingMetricsConsumer does, so you can emit per-task metrics. In my use case, using statsd counters and aggregating metrics component-wide is not an option really, because I want to know per-component instance (i.e. task) metrics of my KafkaTridentSpout (it works by having a separate instance of this spout assigned to a separate Kafka partition), so the metrics aren't summable across tasks.
"metrics.statsd.metric_type" property. Allowable values: "counter" (default) or "gauge".
Type property "metrics.statsd.metric_type"
"metrics.statsd.include_task_id" property. If set to true (default is false), the Statsd metric name will include the Storm Task ID.
Add a new section "Statsd Metric Name" with info on the new configuration property "metrics.statsd.include_task_id".
|
Hi Jason, I've added more changes to the pull request, specifically:
Please review the changes. It would be great if you could merge this into your Regards, Danijel |
|
I've also modified the |
|
Hi, Any chance getting these changes into your branch? Thanks, |
Hi,
This pull request appends the Storm component's task ID to the name of the metric sent to Statsd.
This is needed in cases of Storm component paralellism, because if a Storm component has multiple instances (tasks) running, each task records it own, separate metric, but the current implementation doesn't differentiate the separate tasks metrics, so the metrics get sent to Statsd under the same name, and then are furtherly aggregated by Statsd into a single metric. I think we need to differentiate the different task's metrics, so I've added the task ID to the metric name that gets sent to Statsd.
See Storm's default
LoggingMetricsConsumer, which correctly records separate per-task metrics into the log file.