-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Implement CCS telemetry export as part of _cluster/stats #112310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Documentation preview: |
a447aff
to
e484f09
Compare
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes requested.
Also, this has fewer tests / test changes than I would have guessed. Is ClusterStatsMonitoringDocTests.java
and the docs tests the only ones to change for this?
Ideas on how we might generate other tests that also exercise searches - such as an IT test like you did for populating the CCSUsageTelemetry class?
.../src/main/java/org/elasticsearch/action/admin/cluster/stats/TransportClusterStatsAction.java
Outdated
Show resolved
Hide resolved
==== | ||
|
||
`ccs`:: | ||
(object) Contains information relating to <<modules-cross-cluster-search, cross-cluster search>> settings and activity in the cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I usually also ask the PM to review the end user docs.
cc: @naj-h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the include_remotes option be available to enable these metrics? If so, should we document it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naj-h Yes on both, in the next pull following this one.
We already test the collection part and the JSON rendering part in existing tests, so the new test only verifies the part where it is connected to a specific stats request. I plan to do a bit more roundtrip testing with remote clusters functionality but for this I think most of it is already covered. But if you see parts that are missing we could add more. |
5a78dab
to
9fb55a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds this structure to stats:
with telemetry data about CCS searches.