STORM-3648 add meter to track worker heartbeat rate #3294
Conversation
| @@ -196,6 +198,8 @@ private Object loadWorker(IStateStorage stateStorage, IStormClusterState stormCl | |||
| throws Exception { | |||
| workerState = new WorkerState(conf, context, topologyId, assignmentId, supervisorIfaceSupplier, port, workerId, | |||
| topologyConf, stateStorage, stormClusterState, autoCreds, metricRegistry); | |||
| this.heatbeatMeter = metricRegistry.meter("doHeartbeat-calls", workerState.getWorkerTopologyContext(), | |||
| "__system", -1, "default"); | |||
There was a problem hiding this comment.
It is better to not hardcode it. We can use SYSTEM_COMPONENT_ID, SYSTEM_TASK_ID. And it looks like it doesn't make much sense to have streamId here.
There was a problem hiding this comment.
Would you prefer I pass in "" for streamId? Or are you indicating we should have a metric name convention that allows skipping inclusion of stream?
There was a problem hiding this comment.
I see there is already a metricName function https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java#L244 which doesn't take streamId
There was a problem hiding this comment.
Also I feel like the metrics should have the key before the value, for example, we now have
xx.<compId>.<taskId>.yy.
I think it should be xx.componentId.<compId>.taskId.<taskId>.yy so the values are more readable. But this is a separate issue.
agresch
force-pushed
the
agresch_storm_3648
branch
from
330775c
to
2643f69
Compare
What is the purpose of the change
Add a meter to allow users to track/alert on slowdown of worker heartbeat rate.
How was the change tested
built code.