-
Notifications
You must be signed in to change notification settings - Fork 37
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
Introduce indices filter #167
Conversation
Added Signed-off-by |
Hi @lukas-vlcek , could you please help review again? I missed Signed-off-by in the commit message previously. I reverted the commit, then recommitted with Signed-off-by. |
@tntruong723 The sign-off check requires all commits to be signed. You will need to update the PR to keep only commits with sign-off and then force push ( |
Signed-off-by: nhuttran <truong.nhut_tran.ext@nokia.com>
@lukas-vlcek Thank you for guiding me. I did that. Could you please help review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thanks for your contribution!
I did a bit more deep review and found couple of things we should look at and improve. I am perfectly fine to help here so do not worry if you do not feel like working on all of it. I am here to help.
Given that a new OpenSearch release is around the corner (2.7.0 should be out on April 25th) it would be great if we can manage to get as much finished as possible before we release new version of this plugin for OS 2.7.0. Feel free to let me know which specific tasks you want to look at and I will handle the rest.
Now, let's see...
I think there is a room for improvement:
50_10_nodes_filter.yml
Why is the "Remove persistent settings" part repeated three times?
Move tests to a new yml file
I can see the tests were added to the file that is testing nodes filter feature. I think this is limitting and can be confusing. I suggest creating a new dedicated testing file.
For instance: Create a new file called 60_10_indices_filter.yml
. Then as part of the test (can be in setup/teardown sections) create several indices in the cluster and then test that indices filter really works. For example, create indices a
, b
and c
, then apply filter for a
indices and then test that there are no metrics for indices b
or c
present in the output of the plugin.
Hint: To run tests from the new file only you can use:
./gradlew :yamlRestTest -Dtests.method="test {yaml=/60_10_indices_filter}"
Index filter is for index prefixes only?
I am not sure about the "filter indices statistics with indices starting with prefixes" part. Does the "wildcard(s) in the middle" not work? For example "log*-02-2023"
?
How about this:
# Make sure we delete all indices
curl -X DELETE localhost:9200/_all
# Create some indices for the test
curl -s -H "Content-Type: application/json" -X PUT localhost:9200/log-1-2023
curl -s -H "Content-Type: application/json" -X PUT localhost:9200/log-2-2023
curl -s -H "Content-Type: application/json" -X PUT localhost:9200/log-3-2023
curl -s -H "Content-Type: application/json" -X PUT localhost:9200/log-4-2023
# Verify indices are in place
curl -X GET localhost:9200/_cat/indices
# yellow open log-3-2023 lEmwSHwkSnKgP97LfggCOQ 1 1 0 0 208b 208b
# yellow open log-4-2023 TnK7bQtQQVqEGeuEYbi3wg 1 1 0 0 208b 208b
# yellow open log-1-2023 0a_7ge74QrKO9QNuGzgY0Q 1 1 0 0 208b 208b
# yellow open log-2-2023 qPzZS9m2RRq8p_Za6at4iQ 1 1 0 0 208b 208b
# The following command gets indices stats for all matching indices.
# Notice the filter does not have to follow "prefix-" form only.
curl -X GET "localhost:9200/log-*-2023/_stats"
Enable index filter options
More generally on index filters:
Setting up an index filter can include couple of decisions. Right now the code is opting for a default one (https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/action/support/broadcast/BroadcastRequest.java#L53), which points to STRICT_EXPAND_OPEN_FORBID_CLOSED
(https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/action/support/IndicesOptions.java#L580-L587). If I read the documentation correctly it can return an error if the target index does not exist or is closed.
Let's see what that means:
# Make sure we delete all indices
curl -X DELETE localhost:9200/_all
# Create some indices for the test
curl -s -H "Content-Type: application/json" -X PUT localhost:9200/log-1-2023
curl -s -H "Content-Type: application/json" -X PUT localhost:9200/log-2-2023
curl -s -H "Content-Type: application/json" -X PUT localhost:9200/log-3-2023
curl -s -H "Content-Type: application/json" -X PUT localhost:9200/log-4-2023
curl -X GET localhost:9200/_cat/indices
# yellow open log-3-2023 lEmwSHwkSnKgP97LfggCOQ 1 1 0 0 208b 208b
# yellow open log-4-2023 TnK7bQtQQVqEGeuEYbi3wg 1 1 0 0 208b 208b
# yellow open log-1-2023 0a_7ge74QrKO9QNuGzgY0Q 1 1 0 0 208b 208b
# yellow open log-2-2023 qPzZS9m2RRq8p_Za6at4iQ 1 1 0 0 208b 208b
# This returns an error 404. That is expected.
curl -X GET "localhost:9200/log-5-2023/_stats"
# More problematic case is if the filter contains both valid and invalid (in this case non-existent) indices
# We still get 404 although the index "log-1-2023" actually exists
curl -X GET "localhost:9200/log-5-2023,log-1-2023/_stats"
I think we need to give users some option to control how the index filter is setup. That means some control over IndicesOptions.java
that is used by IndicesStatsRequest
instance.
Nice to have
Checkstyle
This plugin currently does not use any code checkstyle (it is a missing feature, we need to fix it: #9). But if it did then it would probably reject such a long line:
Do you think you can put more formatting on it?
Unexpected input values
I would like to see more tests around unexpected input values, like this:
- do:
cluster.put_settings:
body:
persistent:
prometheus.indices_filter: [ "", null, 10 ]
flat_settings: true
Given that these values come directly from the user input we should make sure we know what happens when the values are "unpredictible". Does it fire an exception? Are they ignored? Are they all converted to a string values?
(Note to myself: We should implement similar test for nodes filter as well.)
Thanks for your review. Let me try to fix all. If I have any issues, I will let you know. |
Signed-off-by: nhuttran <truong.nhut_tran.ext@nokia.com>
Why is the "Remove persistent settings" part repeated three times? Move tests to a new yml file Index filter is for index prefixes only? Checkstyle Unexpected input values Enable index filter options |
Thanks for the update.
indicesOptions = IndicesOptions.lenientExpandOpen();
// or indicesOptions = IndicesOptions.lenientExpand(); // ??
new IndicesStatsRequest().indicesOptions(indicesOptions).indices(...) I am not sure how to deal with closed indices (i.e. |
Signed-off-by: nhuttran <truong.nhut_tran.ext@nokia.com>
I'm done enabling index filter options. Could you please help review it? |
I think we are almost there. Thank you for all your effort! Let me just bring up my last review comments (and sorry for my delay in reviewing, I was travelling a bit last week): 1) About the
|
Thank you for your comments. 1) About the PROMETHEUS_OPTIONS_DESCRIPTION_KEY property 2) About the STRICT_EXPAND_OPEN_HIDDEN_FORBID_CLOSED option |
@tntruong723 Yes you get it right. As for the first point the list of all available indices options can be now overridden from outside (by setting the property Thank you very much! |
Signed-off-by: nhuttran <truong.nhut_tran.ext@nokia.com>
@lukas-vlcek I fixed your review comments. Sorry for the late update. Many things need to be done in my project recently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me. Thank you very much for this contribution!
This is backport of Aiven-Open#167 Signed-off-by: nhuttran <truong.nhut_tran.ext@nokia.com>
This is backport of #167 Signed-off-by: nhuttran <truong.nhut_tran.ext@nokia.com>
This addresses the issue #166