Skip to content

Add source mode stats to MappingStats #117463

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
Merged
merged 11 commits into from
Nov 28, 2024
Merged

Add source mode stats to MappingStats #117463

merged 11 commits into from
Nov 28, 2024

Conversation

@martijnvg
Copy link
Member

No description provided.

@martijnvg martijnvg force-pushed the source_mode_mapping_stats branch from 30d80ce to 00ad196 Compare November 25, 2024 13:27
@martijnvg martijnvg marked this pull request as ready for review November 27, 2024 19:31
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

@martijnvg martijnvg requested a review from dnhatn November 27, 2024 19:38
dnhatn
dnhatn reviewed Nov 27, 2024
View reviewed changes
server/src/main/java/org/elasticsearch/action/admin/cluster/stats/MappingStats.java Outdated
Map<?, ?> sourceFieldDefinition = (Map<?, ?>) map.get("_source");
if (sourceFieldDefinition.containsKey("mode")) {
String mode = (String) sourceFieldDefinition.get("mode");
mappingSourceModeUsageCount.compute(mode.toString().toLowerCase(Locale.ENGLISH), (k, v) -> v == null ? 1 : v + 1);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to account for indices with _source.mode specified twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here we iterate over all unique MappingMetadata instances, so I don't think we see same mappings multiple times? So I don't we account same indices multiple times in mappingSourceModeUsageCount? We can under account, when mappings are identical between indices.

If both index.mappings.source.mode index setting and _source.mode mapping attribute is used then this change picks either the source mode stats or index mode stats.

I'm also ok with just accounting source mode from index.mappings.source.mode index setting, since _source.mode is on its way out and only tech preview synthetic source usages are configured that way. Maybe account _source.mode usages is not worth the complexity?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe account _source.mode usages is not worth the complexity?
++

@martijnvg martijnvg requested a review from dnhatn November 27, 2024 20:19
dnhatn
dnhatn approved these changes Nov 27, 2024
View reviewed changes
Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

🚢 Thanks Martijn!

server/src/main/java/org/elasticsearch/action/admin/cluster/stats/MappingStats.java Outdated
AnalysisStats.countMapping(mappingCounts, indexMetadata);

var sourceMode = SourceFieldMapper.INDEX_MAPPER_SOURCE_MODE_SETTING.get(indexMetadata.getSettings());
sourceModeUsageCount.compute(sourceMode.toString().toLowerCase(Locale.ENGLISH), (k, v) -> v == null ? 1 : v + 1);
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe use map.merge() instead.

@martijnvg martijnvg added the test-full-bwc Trigger full BWC version matrix tests label Nov 28, 2024
@martijnvg martijnvg added the auto-backport Automatically create backport pull requests when merged label Nov 28, 2024
@martijnvg
Copy link
Member Author

The 8.15.6 bwc ci job failed with the following transient failure:

* What went wrong:
Could not determine the dependencies of task ':qa:full-cluster-restart:v8.15.6#bwcTest'.
> Could not resolve all dependencies for configuration ':qa:full-cluster-restart:es_distro_extracted_bwc_8.15.6'.
   > Could not find elasticsearch-distribution:elasticsearch:8.15.6.
     Searched in the following locations:
       - https://artifacts-no-kpi.elastic.co/downloads/elasticsearch/elasticsearch-8.15.6-linux-x86_64.tar.gz
     Required by:
         project :qa:full-cluster-restart

The other 19 bwc ci jobs completed successfully Ignoring this. Ignoring this failure.

@martijnvg martijnvg merged commit 6a4b68d into elastic:main Nov 28, 2024
14 of 17 checks passed
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Nov 28, 2024
@martijnvg
Add source mode stats to MappingStats (elastic#117463)
03eba96
@martijnvg martijnvg mentioned this pull request Nov 28, 2024
Merged
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
❌ 8.17 Commit could not be cherrypicked due to conflicts
✅ 8.x

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 117463

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Nov 28, 2024
@martijnvg
Add source mode stats to MappingStats (elastic#117463)
86dc1dc
@martijnvg martijnvg mentioned this pull request Nov 28, 2024
Merged
martijnvg added a commit that referenced this pull request Nov 28, 2024
@martijnvg
[8.x] Add source mode stats to MappingStats (#117694)
4b3ecd3 
Backporting #117463 to 8.x branch.
elasticsearchmachine pushed a commit that referenced this pull request Nov 28, 2024
@martijnvg
[8.17] Add source mode stats to MappingStats (#117697)
7ed32c2 
* Add source mode stats to MappingStats (#117463)

* update bwc logic for 8.17
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
@martijnvg @alexey-ivanov-es
Add source mode stats to MappingStats (elastic#117463)
68653b1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dnhatn dnhatn dnhatn approved these changes

Assignees

No one assigned

Labels

auto-backport Automatically create backport pull requests when merged >non-issue :StorageEngine/Mapping The storage related side of mappings Team:StorageEngine test-full-bwc Trigger full BWC version matrix tests v8.17.1 v8.18.0 v9.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@martijnvg @elasticsearchmachine @dnhatn