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

Add Indices filter flag #764

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

chapost1
Copy link

@chapost1 chapost1 commented Aug 17, 2023

NOTE: this is a new PR in behalf of #574
as the previous one has DCO issue and after trying to squash commit to sign all of them, it messed the PR so it has been decided to open a new one.

Original Description:
As can be seen earlier, certain people asked for the option to filter (IN) the indices which are exported.

Especially if you have a lot of them and you need to monitor just certain, the rest of it, is just a waste of resources.

ATM, fetching indices settings/stats/mappings queries all the existing ones using this kind of GET requests:
GET /_all/_settings, GET /_all/_stats and GET /_all/_mappings

As mentioned in the docs:
https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-get-settings.html

It's possible to just retrieve the ones which match 2 types of queries:

  1. Matching using wildcard(*) expression. i.e:
    GET /prefix_*/_stats

  2. Matching a list of indices, using comma:
    GET /first_name,second_name/_stats

It is possible to mix them both. ie:
GET /prefix_1_*,prefix_2_*/_stats

The new added flag:
es.indices_selector (default is '_all')

Note: The flag will be used only if any export indices flag is used.
To use it, pass an expression to match your use-case indices, default '_all' just like before.
By passing one of the options mentioned above as a value (statement with a wildcard, list with commas), it will be used in the http request to retrieve statistics.

This change would be very appreciated,
Thanks.

Signed-off-by: Shahar Tal <[email protected]>
@chapost1 chapost1 mentioned this pull request Aug 17, 2023
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.

I think this is pretty straightforward and overall looks pretty good. Thinking about how this is used, I think it would be better to call this an index selector instead of a filter. It's not filtering out indices, it is selecting which ones to get metrics for. That would make the flat es.indices_selector and would mean renaming the variables. Code-wise I think everything is okay.

@chapost1
Copy link
Author

@sysadmind

Thanks for the rename suggestion.
I've modified it accordingly, and updated the PR.

@chapost1
Copy link
Author

@sysadmind
any update?

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.

Just one small fix. Thanks for your patience.

main.go Outdated Show resolved Hide resolved
Modified. es.indices_selector description

Co-authored-by: Joe Adams <[email protected]>
Signed-off-by: Shahar Tal <[email protected]>
@chapost1
Copy link
Author

@sysadmind
Thanks, I've committed your change proposal

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.

2 participants