NodeAllocationStats gives these two more compared to the DESIRED_BALANCE_... metrics above. Not sure if they're needed, but I have exported all of them.

I do not think I am following.

New stats claim they represent the current state of the nodes and are populated from allocationStatsPerNodeRef that is set from nodeAllocationStats or the last argument here.
In its turn the last argument is effectively copied from stats entries that represent the desired state.

How are current stats different from desired stats?

why do you say stats represents desired state? To me it seems to be based on the current cluster state in org.elasticsearch.cluster.routing.allocation.AllocationStatsService.NodeStatsProvider#stats? Either I'm missing something or the naming is confusing! :))

Uh, maybe I am confused. We pass the desiredBalance to the call above but not the current cluster state?

It is just to manage the dependencies to be able to reuse AllocationStatsService. There, we used to pass the entire Allocator only to get the desired balance out of it, which seems to be used for calculating undesired shard count. (git tells me it's your code actually!)

... so if you find a different approach to reuse that code more useful/readable, or see some comments missing or renames, please let me know.

I see. That component was originally created to expose stats via endpoint and was not originally used in reconciler.

The first one (minor) is that it accesses the cluster state via clusterService.state() and not from allocation. They should be the same as this is happening during cluster state update computation, but that is not immediately obvious.

The second one (more substantial) is the set of metric it provides. It does not produce the same metrics as we have for computed balance. This might introduce confusing while comparing them and opens a possibility of publishing different metrics under similar names. Also since this service is used in public API, we will be limited in how we can change it. This might become relevant as we discuss the imbalance measurements and experiment with different balancing functions.

Based on this I suggest not to rely on it and instead implement an apm specific service that is used in both computer and reconciler.

The first one (minor) is that it accesses the cluster state via clusterService.state()

I can change that and make it accept also a state.

It does not produce the same metrics as we have for computed balance.

Not sure I follow. The three node-level metrics from the desired balance that we currently have are shard_count, write_load and disk_usage. We have in this PR those three calculated based on current cluster state. There are two more here, that as I mentioned I chose to export, but we can drop them.

Also since this service is used in public API, we will be limited in how we can change it.

That code is now refactored into its own class. For now we're reusing it because it provides what we need. If we need to later, we can duplicate it, extend it or whatever is appropriate based on what we need.

We discussed this thread offline,

Re

It does not produce the same metrics as we have for computed balance.

It misses weights, but we discussed that they are not necessary for now.
We also discussed the possibility of weight computations diverging. This aspect could be addressed in future when this problem arises.

originally, I had these as es.allocator.allocations.node_ but APM complained that it is too long so I changed to this.

I find it a bit confusing with the inner class for reuse, could we just add an overload to AllocationStatsService that took a desired balance to use in the computation, and everyone just gets given a reference to a single instance of it?

e.g. add

public Map<String, NodeAllocationStats> stats(DesiredBalance desiredBalance) {
   // ...
}

Ahh there would be a circular dependency between allocator and stats service then.

I reckon it'd be clearer if the thing that calculated the stats just became a separate shared piece of infrastructure that took a DesiredBalance from somewhere and returned the stats. The inner class and the interface makes it a bit tricky to read.

Yes, the dependencies here are not straight-forward and largely the reason I came up with this. But how is your suggestion different than what's in the PR? I can move out the inner class and get rid of the interface. Other than that, it is the same, no?

I pushed a5381e1.