Add cluster UUID to Cluster Stats API response

[ycombinator]

Resolves #32205.

Adds cluster_uuid field to GET _cluster/stats REST API response.

[elasticmachine]

Pinging @elastic/es-core-infra

[jasontedor]

I am good with it.

[original-brownbear]

@ycombinator this is failing integration tests

   >                     cluster_uuid: unexpected but found [YjAvIhsCQ9CbjWZb2qJw3Q]

I think you need to add a line - is_true for the cluster_uuid field to rest-api-spec/test/cluster.stats/10_basic.yml to make this pass.

[ycombinator]

Thanks so much @original-brownbear. I kept looking at another unrelated failure in CI!

[ycombinator]

@pickypg I could use your advice on this PR. The goal of this PR is to include cluster_uuid in the Cluster Stats API response alongside the cluster_name field. That way Metricbeat won't have to make an additional API call just to fetch the cluster UUID.

The current CI failure is this:

FAILURE 0.12s J2 | ClusterStatsMonitoringDocTests.testToXContent <<< FAILURES!
   > Throwable #1: org.junit.ComparisonFailure: expected:<...},"cluster_stats":{"[]timestamp":145160640...> but was:<...},"cluster_stats":{"[cluster_uuid":"Z7R4gXE2RvyLDG0ROIP_8g","]timestamp":145160640...>
   >  at __randomizedtesting.SeedInfo.seed([84826F5AB62CCA24:629968E21F2084CF]:0)
   >  at org.elasticsearch.xpack.monitoring.collector.cluster.ClusterStatsMonitoringDocTests.testToXContent(ClusterStatsMonitoringDocTests.java:327)
   >  at java.lang.Thread.run(Thread.java:748)
Completed [17/50] on J2 in 0.19s, 15 tests, 1 failure <<< FAILURES!

I could simply "fix" it by including cluster_uuid inside the cluster_stats object in the cluster stats monitoring doc. But I don't think that's the right fix, especially since cluster_uuid is a top-level field in that monitoring doc already!

One hacky way to fix this would be to somehow remove the cluster_uuid field after this call:

elasticsearch/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/collector/cluster/ClusterStatsMonitoringDoc.java

Line 144 in 6ed1ad0

clusterStats.toXContent(builder, params);

But I'm not even sure if that's possible. And even if it is, it's pretty hacky :(

Another fix, probably the right one, would be to remove this line from my PR:

https://github.com/elastic/elasticsearch/pull/32206/files#diff-7016058c4ba8556b31beb10519607597R121

... and instead insert cluster_uuid into the response body the same way cluster_name is being inserted today, over here:

elasticsearch/server/src/main/java/org/elasticsearch/rest/action/RestActions.java

Line 185 in 6ed1ad0

builder.field("cluster_name", response.getClusterName().value());

But that's a far-reaching change with tons of other classes being touched in the process.

Any ideas on how best to proceed here?

[pickypg]

@ycombinator I faced a nearly identical issue when I made an equivalent change to the GET /_cluster/state endpoint: #30143. FWIW, X-Pack monitoring combines the response from _cluster/stats and _cluster/state into the cluster_stats object, so you may not even need this step, but I think it's appropriate for every cluster API to respond with the UUID.

In that PR, one thing that I did that will probably help you is I disabled the REST test for 6.x until I actually backported the change, then enabled it in a follow-up PR: #30264.

In my case, I did what you are currently doing and I think what you are currently doing is the right approach. I have always thought that it was weird that the cluster name is added by the REST handler.

Either way, you're going to have to fix a few tests that depend on the response from cluster stats, and I think that just is what it is.

[ycombinator]

Thanks @pickypg. I looked at #30143 and took away two things from it for this PR:

1. I should add a rest-api-spec test to assert that the cluster stats API response now contains the cluster_uuid field at the top level in the response JSON. I've tried to address that in bef4ab3.
2. In cluster stats Monitoring docs, it looks like you ended up duplicating the cluster_uuid in the cluster_state object inside the doc as well as at the top-level of the doc. It looks like I'll have to live with the same thing in this PR, that is, we'll now have cluster_uuid show up in a third place in cluster stats Monitoring docs, in the cluster_stats object as well.

Do these seem right to you?

[pickypg]

Sounds great to me. The duplicated cluster_uuid is unfortunate, but we won't index it and thus won't pay much of a penalty for it.

[pickypg]

LGTM

[ycombinator]

Backported to:

• 6.x / 6.5.0: 60583f2