Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce includeShardsStats in the stats request to indicate that we only fetch a summary #100466

Merged
merged 17 commits into from Oct 13, 2023
Merged

Introduce includeShardsStats in the stats request to indicate that we only fetch a summary #100466

merged 17 commits into from Oct 13, 2023

Conversation

NEUpanning
Copy link
Contributor

relates #99744

@elasticsearchmachine elasticsearchmachine added v8.12.0 needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team labels Oct 7, 2023
@NEUpanning NEUpanning mentioned this pull request Oct 7, 2023
Closed
@DaveCTurner DaveCTurner added the :Data Management/Stats Statistics tracking and retrieval APIs label Oct 9, 2023
@DaveCTurner DaveCTurner self-assigned this Oct 9, 2023
@elasticsearchmachine elasticsearchmachine added Team:Data Management Meta label for data/management team and removed needs:triage Requires assignment of a team area label labels Oct 9, 2023
@elasticsearchmachine
Copy link
Collaborator

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

DaveCTurner
DaveCTurner reviewed Oct 9, 2023
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basic idea seems good although I don't think it quite works as written yet.

server/src/main/java/org/elasticsearch/indices/IndicesService.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequest.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequest.java Outdated
@@ -33,6 +34,7 @@ public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {

private CommonStatsFlags indices = new CommonStatsFlags();
private final Set<String> requestedMetrics = new HashSet<>();
private boolean needShardsStats = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming nit/suggestion:

Suggested change
private boolean needShardsStats = true;
private boolean includeShardsStats = true;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. Fixed in 373cc6f

@DaveCTurner
Copy link
Contributor

@elasticmachine test this please

DaveCTurner
DaveCTurner reviewed Oct 9, 2023
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks that looks better. Now this needs some tests showing that we do/don't drop the shard-level stats depending on the flag.

I'll trigger another CI run to see if it picks up anything I missed.

I left some inline nits too.

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesStatsAction.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/indices/NodeIndicesStatsTests.java Outdated Show resolved Hide resolved
...ugin/core/src/test/java/org/elasticsearch/xpack/core/DataTiersUsageTransportActionTests.java Show resolved Hide resolved
...test/java/org/elasticsearch/xpack/monitoring/collector/node/NodeStatsMonitoringDocTests.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/indices/NodeIndicesStats.java Outdated Show resolved Hide resolved
@DaveCTurner
Copy link
Contributor

@elasticmachine test this please

NEUpanning and others added 6 commits October 9, 2023 18:34
@NEUpanning @DaveCTurner
Update server/src/main/java/org/elasticsearch/rest/action/admin/clust…
5f445cd 
…er/RestNodesStatsAction.java

Co-authored-by: David Turner <david.turner@elastic.co>
@NEUpanning @DaveCTurner
Update server/src/main/java/org/elasticsearch/indices/NodeIndicesStat…
9f9a757 
…s.java

Co-authored-by: David Turner <david.turner@elastic.co>
@NEUpanning @DaveCTurner
Update x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack…
c2d699c 
…/monitoring/collector/node/NodeStatsMonitoringDocTests.java

Co-authored-by: David Turner <david.turner@elastic.co>
@NEUpanning @DaveCTurner
Update x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/…
431cf3d 
…DataTiersUsageTransportActionTests.java

Co-authored-by: David Turner <david.turner@elastic.co>
@NEUpanning @DaveCTurner
Update server/src/test/java/org/elasticsearch/indices/NodeIndicesStat…
100f568 
…sTests.java

Co-authored-by: David Turner <david.turner@elastic.co>
@NEUpanning
add a test
8543023
@NEUpanning
Copy link
Contributor Author

I added some tests in 8543023

@NEUpanning NEUpanning changed the title Introduce needShardsStats in the stats request to indicate that we only fetch a summary Introduce includeShardsStats in the stats request to indicate that we only fetch a summary Oct 9, 2023
DaveCTurner
DaveCTurner reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one substantive request about exposing the whole stats map vs testing it by accessing the stats for a single index.

server/src/main/java/org/elasticsearch/indices/NodeIndicesStats.java Outdated
Comment on lines 217 to 218
// for testing
Map<Index, List<IndexShardStats>> getStatsByShard() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we test this via getShardStats(Index index) instead please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we test this via getShardStats(Index index) in 3ddf821

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesStatsAction.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/TransportVersions.java Outdated
@@ -136,7 +136,7 @@ static TransportVersion def(int id) {
public static final TransportVersion ML_PACKAGE_LOADER_PLATFORM_ADDED = def(8_512_00_0);
public static final TransportVersion PLUGIN_DESCRIPTOR_OPTIONAL_CLASSNAME = def(8_513_00_0);
public static final TransportVersion UNIVERSAL_PROFILING_LICENSE_ADDED = def(8_514_00_0);

public static final TransportVersion NEED_SHARDS_STATS_ADDED = def(8_515_00_0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming nit

Suggested change
public static final TransportVersion NEED_SHARDS_STATS_ADDED = def(8_515_00_0);
public static final TransportVersion INCLUDE_SHARDS_STATS_ADDED = def(8_515_00_0);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's replaced in 9d20eda

@DaveCTurner
Copy link
Contributor

@elasticmachine test this please

@DaveCTurner
Copy link
Contributor

The CI failures all seem to relate to the branch being stale, you'll just need to merge main and fix up the TransportVersions conflict again.

@NEUpanning
Copy link
Contributor Author

The TransportVersions conflict resolved in d11faa4. Ready to be tested again.

DaveCTurner
DaveCTurner approved these changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DaveCTurner
Copy link
Contributor

@elasticmachine test this please

@DaveCTurner DaveCTurner added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 13, 2023
@NEUpanning
Copy link
Contributor Author

I think we could reasonably indicate includeShardsStats = true in _cat/nodes and _cat/allocation etc since these APIs also don't use NodeIndicesStats#statsByShard. I would like to open another PR for this.

@NEUpanning
Copy link
Contributor Author

NEUpanning commented Oct 13, 2023 •
edited

According to the CI failure message, it seems that includeShardsStats should be true in DataTiersUsageTransportActionTests.buildNodeStats.

@DaveCTurner DaveCTurner removed the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 13, 2023
@DaveCTurner
Copy link
Contributor

it seems that includeShardsStats should be true in DataTiersUsageTransportActionTests.buildNodeStats.

Ah yes indeed it should be. See also #100230, because it'd be better for this action not to use node stats at all.

@DaveCTurner
Copy link
Contributor

@elasticmachine test this please

@DaveCTurner DaveCTurner added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 13, 2023
@elasticsearchmachine elasticsearchmachine merged commit 9003cbe into elastic:main Oct 13, 2023
15 checks passed
@NEUpanning NEUpanning mentioned this pull request Oct 17, 2023
Merged
@NEUpanning
Copy link
Contributor Author

I think we could reasonably indicate includeShardsStats = true in _cat/nodes and _cat/allocation etc since these APIs also don't use NodeIndicesStats#statsByShard. I would like to open another PR for this.

I have finished it in #100938.

elasticsearchmachine pushed a commit that referenced this pull request Nov 23, 2023
@NEUpanning
Set includeShardsStats = false in NodesStatsRequest where the caller …
6cf4c80 
…does not use shards-level statistics (#100938)

Relates #100466
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaveCTurner DaveCTurner DaveCTurner approved these changes

Assignees

@DaveCTurner DaveCTurner

Labels
auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Data Management/Stats Statistics tracking and retrieval APIs >enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Data Management Meta label for data/management team v8.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@NEUpanning @elasticsearchmachine @DaveCTurner