Introduce node mappings stats #89807

kingherc · 2022-09-05T15:19:58Z

In CommonStats so that they are visible in NodeIndicesStats only at the node and index (but not shard) levels. Also visible in the _cat/nodes table.

Relates to issue #86639

github-actions · 2022-09-05T15:20:10Z

Documentation preview:

✨ Changed pages

DaveCTurner · 2022-09-06T08:10:23Z

Fixes #86639 partly

Github doesn't understand the partly bit and will close #86639 when this PR is merged. Best to just say Relates instead of Fixes

elasticsearchmachine · 2022-09-06T18:09:27Z

Pinging @elastic/es-distributed (Team:Distributed)

elasticsearchmachine · 2022-09-06T18:09:27Z

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

elasticsearchmachine · 2022-09-06T18:10:11Z

Hi @kingherc, I've created a changelog YAML for you.

kingherc · 2022-09-07T09:21:58Z

@original-brownbear I'm removing you from reviewers for now, and converting this back to draft, since we want more stuff on the sub-PR #89798. Once that is done, I'll rebase this and re-invite you to review.

So that they are visible in NodeIndicesStats only at the node and index (but not shard) levels. Also visible in the _cat/nodes table. Relates to issue elastic#86639

And make an exact count yaml REST test

And FieldMapperStats to NodeMappingStats

kingherc · 2022-09-08T17:41:33Z

Hi @original-brownbear , I rebased on master and did some fixes. Feel free to review now, thanks!

server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java

And use in checkFieldLimit

DaveCTurner

I mainly just looked at the docs changes and left some suggestions.

DaveCTurner · 2022-09-09T12:39:08Z

docs/reference/how-to/size-your-shards.asciidoc

+per index held by each data node. In a running cluster, you can also consult the
+<<cluster-nodes-stats,Nodes stats API>>'s `mappings` indices statistic, which
+reports the number of field mappings and an estimation of their heap overhead.


I think we should consider rephrasing this whole section in terms of these stats now that they're available. We can however do that in a followup - it's the third item on the list for #86639.

Sure, feel free to gather ideas already here if you'd like. cc @original-brownbear

I'm fine with a follow-up, linking from here is fine by me for now.

docs/reference/cluster/nodes-stats.asciidoc

kingherc · 2022-09-09T12:48:56Z

I mainly just looked at the docs changes and left some suggestions.

Yes feel free to also look at the other changes as well if you'd like, otherwise I consider @original-brownbear as the main reviewer.

kingherc · 2022-09-13T08:56:35Z

Ping @original-brownbear for a review, in case we can make this before FF of 8.5.0.

original-brownbear

A few initial points, I'll give it another pass a little later. Looks good overall though thanks!

docs/reference/cluster/nodes-stats.asciidoc

original-brownbear · 2022-09-15T08:39:36Z

docs/reference/how-to/size-your-shards.asciidoc

+per index held by each data node. In a running cluster, you can also consult the
+<<cluster-nodes-stats,Nodes stats API>>'s `mappings` indices statistic, which
+reports the number of field mappings and an estimation of their heap overhead.


I'm fine with a follow-up, linking from here is fine by me for now.

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/nodes.stats/11_indices_metrics.yml

server/src/main/java/org/elasticsearch/indices/IndicesService.java

server/src/main/java/org/elasticsearch/indices/NodeIndicesStats.java

original-brownbear

Thanks for the changes @kingherc I added a few more questions about the code.

server/src/main/java/org/elasticsearch/action/admin/indices/stats/CommonStats.java

kingherc · 2022-09-15T15:51:45Z

Hi @original-brownbear , I fixed the last comments, feel free to give it another review! thanks!

original-brownbear

Thanks Iraklis, this looks good now. Just one spot that I found where we could massively simplify this for now when reading over it again and some small stuff.

server/src/main/java/org/elasticsearch/action/admin/indices/stats/CommonStats.java

kingherc · 2022-09-19T11:11:14Z

@original-brownbear , fixed your comments. Feel free to check the last commit, thanks!

original-brownbear

LGTM, just some minor NITs that don't require another round of reviewing.

original-brownbear · 2022-09-19T11:15:11Z

server/src/main/java/org/elasticsearch/action/admin/indices/stats/CommonStats.java

+        if (stats.getNodeMappings() != null) {
+            if (nodeMappings == null) {
+                nodeMappings = new NodeMappingStats();
+                nodeMappings.add(stats.getNodeMappings());


Can't we just do what we do for shard stats above and go with:

if (nodeMappings == null) { nodeMappings = stats.getNodeMappings(); } else { nodeMappings.add(stats.getNodeMappings()); }

?

I believe shard stats is doing it very differently than the code above it, which actually creates a new object rather than assigning the reference of the added object (like what shard stats code is doing). So I decided to follow the above code, which seems better/safer to me. Feel free to tell me if you disagree and I can make it like shard stats

Hah yea we have both versions. It doesn't really matter I guess, fine by me to go with the safe version here.

server/src/main/java/org/elasticsearch/indices/IndicesService.java

server/src/main/java/org/elasticsearch/indices/NodeIndicesStats.java

server/src/main/java/org/elasticsearch/index/mapper/NodeMappingStats.java

server/src/main/java/org/elasticsearch/action/admin/indices/stats/CommonStatsFlags.java

elasticsearchmachine added the v8.5.0 label Sep 5, 2022

kingherc force-pushed the feature/86639-new-sizing-stats branch 4 times, most recently from d4ba2f3 to 8375f37 Compare September 5, 2022 17:17

DaveCTurner mentioned this pull request Sep 6, 2022

Report stats related to new sizing guidance #86639

Closed

3 tasks

kingherc force-pushed the feature/86639-new-sizing-stats branch from b909f1e to f4cf6c9 Compare September 6, 2022 09:47

kingherc marked this pull request as ready for review September 6, 2022 18:08

kingherc added :Data Management/Stats Statistics tracking and retrieval APIs :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Sep 6, 2022

elasticsearchmachine added Team:Data Management Meta label for data/management team Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. labels Sep 6, 2022

kingherc requested a review from original-brownbear September 6, 2022 18:09

kingherc added the >enhancement label Sep 6, 2022

kingherc changed the title ~~Introduce FieldMapperStats in CommonStats~~ Introduce FieldMappingStats in CommonStats Sep 6, 2022

kingherc removed the request for review from original-brownbear September 7, 2022 09:21

kingherc marked this pull request as draft September 7, 2022 09:21

kingherc force-pushed the feature/86639-new-sizing-stats branch from 77cdd79 to c60288e Compare September 7, 2022 11:13

kingherc changed the title ~~Introduce FieldMappingStats in CommonStats~~ Introduce node field mapping stats Sep 7, 2022

kingherc force-pushed the feature/86639-new-sizing-stats branch from c60288e to c133985 Compare September 8, 2022 16:33

kingherc changed the title ~~Introduce node field mapping stats~~ Introduce node mappings stats Sep 8, 2022

kingherc added 3 commits September 8, 2022 19:38

Introduce node field mapping stats

2ba7c33

So that they are visible in NodeIndicesStats only at the node and index (but not shard) levels. Also visible in the _cat/nodes table. Relates to issue elastic#86639

Consider also object mappers in counting

135cb89

And make an exact count yaml REST test

Rename field_mappings to mappings in node stats

27d464d

And FieldMapperStats to NodeMappingStats

kingherc force-pushed the feature/86639-new-sizing-stats branch from c133985 to 2821d2c Compare September 8, 2022 16:39

kingherc marked this pull request as ready for review September 8, 2022 17:40

kingherc requested a review from original-brownbear September 8, 2022 17:40

javanna reviewed Sep 9, 2022

View reviewed changes

server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java Show resolved Hide resolved

Rename to getTotalFieldsCount

8106602

And use in checkFieldLimit

kingherc requested a review from javanna September 9, 2022 12:35

DaveCTurner reviewed Sep 9, 2022

View reviewed changes

Fix comments from PR

df7184e

original-brownbear reviewed Sep 15, 2022

View reviewed changes

kingherc self-assigned this Sep 15, 2022

kingherc added 2 commits September 15, 2022 13:34

Fix PR comments

52ddbc1

Merge branch 'main' into feature/86639-new-sizing-stats

3a13df5

kingherc requested a review from original-brownbear September 15, 2022 10:35

original-brownbear reviewed Sep 15, 2022

View reviewed changes

kingherc added 2 commits September 15, 2022 17:54

Fix PR comments

421771d

Fix PR comments

2f47e2c

kingherc requested a review from original-brownbear September 15, 2022 15:51

original-brownbear reviewed Sep 19, 2022

View reviewed changes

Fix PR comments

8bc90ec

kingherc requested a review from original-brownbear September 19, 2022 10:31

original-brownbear approved these changes Sep 19, 2022

View reviewed changes

Fix more PR comments

760cfda

kingherc merged commit 3ed7a04 into elastic:main Sep 19, 2022

kingherc deleted the feature/86639-new-sizing-stats branch September 19, 2022 12:47

DaveCTurner mentioned this pull request Jul 11, 2023

Add shard_stats.total_count column description to /_cat/nodes docs. #97549

Merged

Introduce node mappings stats #89807

Introduce node mappings stats #89807

Uh oh!

Conversation

kingherc commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 5, 2022

Uh oh!

DaveCTurner commented Sep 6, 2022

Uh oh!

elasticsearchmachine commented Sep 6, 2022

Uh oh!

elasticsearchmachine commented Sep 6, 2022

Uh oh!

elasticsearchmachine commented Sep 6, 2022

Uh oh!

kingherc commented Sep 7, 2022

Uh oh!

kingherc commented Sep 8, 2022

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

kingherc Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

original-brownbear Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kingherc commented Sep 9, 2022

Uh oh!

kingherc commented Sep 13, 2022

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

original-brownbear Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kingherc commented Sep 15, 2022

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kingherc commented Sep 19, 2022

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

kingherc Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

original-brownbear Sep 19, 2022

kingherc commented Sep 5, 2022 •

edited

Loading