allocation: add duration and count metrics for write load hotspot#138465
allocation: add duration and count metrics for write load hotspot#138465schase-es merged 14 commits intoelastic:mainfrom
Conversation
The WriteLoadConstraintMonitor now reports as APM metrics its current count of hotspot nodes, and the duration of each hotspot. When a node is reported as hotspotting, this start time is stored along with its node id. When it stops, the time difference between then and now is recorded as a duration metric. Closes: ES-13381
schase-es
added
:Distributed/Allocation
elasticsearchmachine
added
the
Team:Distributed Coordination (obsolete)
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
|
Hi @schase-es, I've created a changelog YAML for you. |
|
|
||
| private List<LongWithAttributes> getHotspotNodesCount() { | ||
| return List.of(new LongWithAttributes(hotspotNodesCount)); | ||
| } |
There was a problem hiding this comment.
We want to return an empty list here if we're not the master, otherwise we'll get a bunch of bogus values each metric interval from the non-master nodes.
Perhaps getHotSpotNodesCount could switch it to -1 and then we could only report if it's been switched back to an actual value (the InternalClusterInfoService stops refreshing the cluster info when the node is no longer master so this approach would naturally only report metrics from the master).
Or any other approach you think is best.
There was a problem hiding this comment.
I don't think we need to make this a cluster state listener, we will stop receiving new ClusterInfo when we are no longer master.
If we make this listen to the cluster state, it adds more dependencies and will add a tiny additional cost to the cluster state applier thread (negligible but we should try and do that as little as possible).
I think I prefer my suggestion to only return a hot-spot node count if we've calculated one since last time we were polled.
There was a problem hiding this comment.
That would also be presumably simpler to test (just confirm we only return the count if onNewInfo has been called since last time we asked)
| ); | ||
|
|
||
| final Set<String> firstWaveHotspotNodes = testState.hotspotNodeIds(); | ||
| final String removeHotspotId = randomSubsetOf(1, testState.hotspotNodeIds()).get(0); |
There was a problem hiding this comment.
Nit: I think you can use randomFrom(Collection<T>)
|
|
||
| // check the test state hotspots are recorded in the counter | ||
| recordingMeterRegistry.getRecorder().collect(); | ||
| assertMetricsCollected(recordingMeterRegistry, List.of((long) testState.hotspotNodeIds().size()), List.of()); |
There was a problem hiding this comment.
I kinda think we shouldn't add the assertions around metrics to every test, just to keep those tests focused on the specific aspects that they're testing? I'd be happy to keep it all for the dedicated test that you have at the bottom.
nicktindall
left a comment
nicktindall
left a comment
There was a problem hiding this comment.
looking good with a few comments
| hotspotDurationHistogram.record(hotspotDuration / 1000.0); | ||
| } | ||
| hotspotNodesCount = hotspotNodeStartTimes.size(); | ||
| hotspotNodesCountUpdatedSinceLastRead = true; |
There was a problem hiding this comment.
I wonder if it'd be feasible to break this stuff out into a method, just so it doesn't distract too much from the monitoring logic?
Likewise with the recording of the start times?
| if (state.term() != hotspotNodeStartTimesLastTerm) { | ||
| hotspotNodeStartTimesLastTerm = state.term(); | ||
| hotspotNodeStartTimes.clear(); | ||
| } |
There was a problem hiding this comment.
Perhaps we can also clear the state if e.g. state. isLocalNodeElectedMaster() == false, this can happen before the term is bumped.
|
|
||
| public WriteLoadConstraintMonitor( | ||
| public static final String HOTSPOT_NODES_COUNT_METRIC_NAME = "es.allocator.allocations.node.write_load_hotspot.current"; | ||
| public static final String HOTSPOT_DURATION_METRIC_NAME = "es.allocator.allocations.node.write_load_hotspot.duration.histogram"; |
There was a problem hiding this comment.
Nit: I think we usually put constants above the fields
| public static final String HOTSPOT_DURATION_METRIC_NAME = "es.allocator.allocations.node.write_load_hotspot.duration.histogram"; | ||
|
|
||
| private volatile long hotspotNodesCount = 0; // metrics source of hotspotting node count | ||
| private volatile boolean hotspotNodesCountUpdatedSinceLastRead = false; // turns off metrics when not master/onNewInfo isn't called |
There was a problem hiding this comment.
Could we maybe achieve the same with a single atomic field instead e.g.
writer:
hotSpotNodesCount.set(count);
reader:
int count = hotSpotNodesCount.getAndSet(-1)
if (count >= 0) {
return List.of(LongWithAttributes(count, ...);
} else {
return List.of();
}
or is there an advantage to having the two fields?
3 participants
The WriteLoadConstraintMonitor now reports as APM metrics its current count of hotspot nodes, and the duration of each hotspot. When a node is reported as hotspotting, this start time is stored in a table along with its node id. When it stops, the time difference between then and now is recorded as a duration metric. The size of this table is the current hotspot count.
Closes: ES-13381