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.

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

Fix index field count #675

Merged

Conversation

hartfordfive
Copy link
Contributor

This change applies a small modification that adds 1 to the field count prior to recursively counting the other nested fields in the mapping. (Issue #674)

Copy link
Contributor

@sysadmind sysadmind left a comment

Choose a reason for hiding this comment

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

You're missing the DCO. The check has instructions for how to fix.
I'm not sure this takes into account a situation where both type and properties are set. I'm not sure if these are mutually exclusive, but if not these would potentially double count those fields.

CHANGELOG.md Outdated Show resolved Hide resolved
VERSION Outdated Show resolved Hide resolved
@hartfordfive
Copy link
Contributor Author

hartfordfive commented Jan 21, 2023

@sysadmind , thank you for the quick feedback. I will address those required changes you pointed out.

As for counting a mapping where both type and property are set, although the type object could be technically be present with the properties field, it isn't usually defined explicitly (at least based on my experience) as that's the default value as per the documentation.

You are not required to set the field type to object explicitly, as this is the default value.

Having said that, the possibility is a valid edge case that should be accounted for, so I'll go ahead and make the modification to take that into account. Thank you for bringing that up.

…indices_mappings feature

Signed-off-by: Alain Lefebvre <hartfordfive@gmail.com>
Signed-off-by: Alain Lefebvre <hartfordfive@gmail.com>
… field is set to object as it will be counted in the recursion case

Signed-off-by: Alain Lefebvre <hartfordfive@gmail.com>
Signed-off-by: Alain Lefebvre <hartfordfive@gmail.com>
Copy link
Contributor

@sysadmind sysadmind left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

@sysadmind sysadmind merged commit 6934773 into prometheus-community:master Jan 22, 2023
SuperQ added a commit that referenced this pull request Jun 29, 2023
BREAKING CHANGES:

The flag `--es.cluster_settings` has been renamed to `--collector.clustersettings`.

* [CHANGE] Rename --es.cluster_settings to --collector.clustersettings
* [FEATURE] Add ILM metrics #513
* [ENHANCEMENT] Add ElasticCloud node roles to role label #652
* [ENHANCEMENT] Add ability to use AWS IAM role for authentication #653
* [ENHANCEMENT] Add metric for index replica count #483
* [BUGFIX] Set elasticsearch_clusterinfo_version_info guage to 1 #728
* [BUGFIX] Fix index field counts with nested fields #675

---------

Signed-off-by: Joe Adams <github@joeadams.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants