Expose index health and status to the _stats API

[gmarouli]

Expose the index health and status to the _stats API. We only enrich the IndicesStatsResponse with the new fields when it is called by the API in order to not affect the performance during internal usage.

Resolve #80413

[dakrone]

Thanks for working on this Mary! I left some comments about the implementation

[dakrone]

This isn't serialized in the constructor and writeTo methods, so it's going to be null quite frequently.

I think we have at least two alternative options:

The first is to continue to pass the cluster state into this method, but construct a map of indexName -> state & health, which is serialized in the IndicesStatsResponse so that it's available on every node. Constructing this map would happen in the constructor, so that we don't hold on to a reference to the cluster state in the class itself.

The second would be to push the health and state into the ShardStats object itself, so that we could collate the index health and open/closed state from the ShardStats the way we calculate other states in getIndices().

I think I lean slightly towards the second option, because it would allow us to expose this information (state and health) in the shard stats API as well, re-using it for this particular use case. What do you think?

[gmarouli]

I was having hard timing deciding how to approach it as well. That's why this draft contains the least invasive option to start with.

I agree these are the most obvious alternatives, I had the following concerns:

1) Keep a map with index state and health
Pros:

1. Minimal penalty during serialization, it's just a map with the metadata
2. This feels to me like the most logical place

Cons:

1. Calculation overhead in the constructor since we would have to go through the shards to determine the relevant indices

2) Add it to the ShardStats
Pros:

1. Nicer code wise and the new information will be available right where we need it, no need for extra maps etc.

Cons:

1. This feels more "convenient" than "right" to me. What I mean with that is, an index has one or more shards, the ShardStats class contains information about a single shard (if I am not mistaken), adding index information feels like we are stretching the scope of the ShardStats to contain also some index information.

I would like to put out there one more option, again with some trade offs:

3) Initialize indices in the constructor and add it in the serialization
Pros:

1. No intermediary maps, the information is available right where we need it
2. This feels to me like the most logical place

Cons:

1. Calculation overhead in the constructor since we would have to go through the shards to determine the relevant indices (if we need this information often then that's not a big deal because we go through the shards once.
2. Serialization penalty, with this option we are increasing the serialized object significantly.

What do you think? Based on this I am kind of leaning towards the first or the third.

[gmarouli]

I gave it a try and implemented the extra maps because it felt the best option. Re-reading your initial comment, you say:

it would allow us to expose this information (state and health) in the shard stats API as well, re-using it for this particular use case

This is not covered in this solution. Do you think this benefit is strong enough to justify adding to the stats of a single shards information about the whole index?

[gmarouli]

@dakrone Thank you for the comments, they were really helpful for me!

[gmarouli]

@elasticmachine update branch

[dakrone]

Thanks Mary! I think this is a better implementation. I left a couple comments and answers to your comment here (because Github won't let me attach a comment to your response for some reason):

2) Add it to the ShardStats
Cons:

1. This feels more "convenient" than "right" to me. What I mean with that is, an index has one or more shards, the ShardStats class contains information about a single shard (if I am not mistaken), adding index information feels like we are stretching the scope of the ShardStats to contain also some index information.

Well, a shard does itself have the concept of state (open or closed), though it's confusing to re-use IndexMetadata.State for a shard, since it says index metadata and not shard metadata. We don't have an enum/state at the shard level because we don't really expose it outside of the concept of an index (yet).

As for health, we have the concept of assigned, initializing, or unassigned for a shard, which very roughly maps on to the index health (I guess it would be either red or green in all cases), so it's almost-but-not-quite pertinent to a shard in addition to an index.

I think it's fine to wait on this sort of fine-grained state for a single shard until we figure out exactly what we want from the "fine-grained health API" project.

3) Initialize indices in the constructor and add it in the serialization
Pros:

1. No intermediary maps, the information is available right where we need it
2. This feels to me like the most logical place

Cons:

1. Calculation overhead in the constructor since we would have to go through the shards to determine the relevant indices (if we need this information often then that's not a big deal because we go through the shards once.
2. Serialization penalty, with this option we are increasing the serialized object significantly.

I agree this is the most logical from a behavior point of view, however, I am also concerned about the overhead for serialization and construction. I took a look at the git-blame output for this and apparently it wasn't a performance optimization added later —it's been implemented that way (calculated on getIndices()) the entire time.

What do you think? Based on this I am kind of leaning towards the first or the third.

I agree, I also would lean towards the first or the third. I think the indices stats API is hit fairly heavily from the monitoring side, so for that reason, I think we should stick with the first option for now. We can always change it later since it is an implementation detail and wouldn't break anything to change.

[dakrone]

When do we expect the cluster state to be null? As far as I can tell it would only be in the tests.

I think we should require it to be non-null (with Objects.requireNonNull(clusterState) and handle the case when clusterState.getMetadata().index(index) returns null gracefully. Then in tests if needed you can pass ClusterState.EMPTY_STATE if you don't need to construct it for anything.

[gmarouli]

Fixed

[gmarouli]

@elasticmachine update branch

[gmarouli]

Thanks for thinking along with me, I am looking forward to see how the fine-grained health api is going to be formed!

[elasticmachine]

Pinging @elastic/es-data-management (Team:Data Management)

[dakrone]

LGTM, thanks for iterating on this Mary!