Track more snapshot-releated node-level stats #130301
Conversation
elasticsearchmachine
added
the
serverless-linked
nicktindall
added
>non-issue
:Distributed Coordination/Snapshot/Restore
…ot_stats_as_metrics
…ot_stats_as_metrics
…ot_stats_as_metrics
…ot_stats_as_metrics
…ot_stats_as_metrics # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
…ot_stats_as_metrics # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
| + ", startTime=" | ||
| + startTime | ||
| + startTimeMillis | ||
| + ", totalTime=" | ||
| + totalTime | ||
| + totalTimeMillis |
There was a problem hiding this comment.
nit: could rename the labels here too
There was a problem hiding this comment.
Resolved in 9bea854 and also added units to the immutable copy object
| this(entries, stateSummaries.v1(), stateSummaries.v2()); | ||
| } | ||
|
|
||
| private static Tuple<Map<State, Integer>, Map<ShardState, Integer>> calculateStateSummaries(List<Entry> entries) { |
There was a problem hiding this comment.
Hmm I think this means we do this computation on every node now which seems wasteful. Could we do it in SnapshotsService still, just on the master?
When I suggested doing this in applyClusterState I meant just updating the existing stats according to the new cluster state, not computing everything from scratch. If we have to do it from scratch every time then I guess it'd be better to happen on the stats-collection thread rather than the cluster applier. At least we could cache the results assuming they won't change before the next stats collection?
There was a problem hiding this comment.
Ahh right yes, that is wasteful. My thinking was to avoid changing the serialization (hence deriving the stats in the ByRepo constructor), and I thought that for many cluster state updates the bulk of the structure doesn't change, so we wouldn't create ByRepo instances unless there was a change, but even despite that we still do quite a bit of unnecessary work.
I changed this in 87864ec to calculate the stats on the metric thread, and only re-calculate them when the cluster state changed. I also added some more logic in 74c3427 to only recalculate the metrics when the SnapshotsInProgress instance changes, because I think this will avoid re-calculating the metrics every time something unrelated in the cluster state changes.
I'm assuming the object-equality-on-no-change thing holds for the customs as well. The check against cluster state version may now be redundant, but I guess it might save fetching the SnapshotsInProgress sometimes.
I don't think we can do this more cleverly in the applier because SnapshotsInProgress seems to lack the ability to query what changed like some of the other cluster state implements, but I may be missing something. I think by doing all the caching and recalculation on the metrics thread we can also avoid making the SnapshotStats field volatile, but if we invalidate it from the applier thread it would have to be.
Edit: some related tidying in 346c15f
…ot_stats_as_metrics
…s.Copy fields/toString
| @@ -178,6 +169,16 @@ public void awaitIdle() { | |||
|
|
|||
| } | |||
|
|
|||
| @Override | |||
| public LongWithAttributes getShardSnapshotsInProgress() { | |||
| return null; | |||
There was a problem hiding this comment.
Do we want to throw here too?
| return null; | |
| throw createUnknownTypeException(); |
If not, could we have a comment saying why we return null?
There was a problem hiding this comment.
My thinking was we may end up with an unknown type repository alongside other valid repositories, and throwing an exception here might interfere with metric collection for the valid repos (I'm not sure how long that state could persist). Returning null signifies that the repository doesn't track shard snapshots in progress so it's effectively a no-op.
If that makes sense I'll add a comment to that effect?
| // Can't get shards for clone entry | ||
| continue; | ||
| } | ||
| for (ShardSnapshotStatus shardSnapshotStatus : entry.shards().values()) { |
There was a problem hiding this comment.
Ok AIUI we now only do this big nested loop when updating the stats cache, which happens when collecting stats from APM and therefore can be disabled by turning off APM right?
There was a problem hiding this comment.
Yes it'll only happen
- when either "shards by state" or "snapshots by state" metrics is requested
- AND the cluster state UUID is not the same as the one used to last calculate these numbers
- AND the snapshots in progress custom is not instance equal to the one used to last calculate these numbers
It should be calculated on the metrics thread and disabling APM will stop it occurring. I could add a dynamic setting to disable these specific metrics if we want to be risk averse.
# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
…ot_stats_as_metrics
Adds additional snapshot metrics and publishes them via APM
Apologies for the size of this change, but most of it is plumbing.
The change itself is quite small.Relates: ES-12055, ES-11927