Introduce indices filter

[tntruong723]

This addresses the issue #166

[tntruong723]

Added Signed-off-by

[tntruong723]

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.

[lukas-vlcek]

@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 (git push -f <PR>) to override the history in the PR.
Or I can do it for you, I think it should keep the attribution to your work (i.e. you should still be listed as an author or co-author).

[tntruong723]

@lukas-vlcek Thank you for guiding me. I did that. Could you please help review?

[lukas-vlcek]

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?

See https://github.com/aiven/prometheus-exporter-plugin-for-opensearch/pull/167/files#diff-ec6e280e1de3b058a2552cc1218b07c8372b21eacf83873b3f9605ab9a1200ecR78

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:

https://github.com/aiven/prometheus-exporter-plugin-for-opensearch/pull/167/files#diff-bd65a2aed3608cb97a456bbd9552ad161128ebe0fd8b4d2174ed8d055c9f2f98R136

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.)

[tntruong723]

Thanks for your review. Let me try to fix all. If I have any issues, I will let you know.

[tntruong723]

Why is the "Remove persistent settings" part repeated three times?
Actually, my patch is for version 2.2.1.0 on our side. When I applied the patch to the main branch, there were some conflicts in other files. I tried to fix them. I remember there were no conflicts in 50_10_nodes_filter.yml, so I did not relook at it carefully.
It's a learn to me. Need to review carefully even if there are no conflicts.

Move tests to a new yml file
I moved tests to 60_10_indices_filter.yml and added some more tests.
Actually, I did add tests in 50_10_nodes_filter.yml on purpose because I was afraid about yamlRestTest before. But thank to this review, I tried to study it and found that it's not hard as my thought. Thanks for this comment.

Index filter is for index prefixes only?
Thanks for this also. Previously, I only thought about filtering indices statistics with indices starting with prefixes. When I was done with the change, I still only thought about this.
I need to open my mind more :). Many learnings from your comments. Thanks, again.

Checkstyle
I added this. But when I built locally there were many warning messages for checking style. I disabled it by commenting out the line "apply plugin: 'checkstyle'" in build.gradle .
I think we need to review Checkstyle rules before applying. It will take time so I think we can do it later. What do you think?
Please note that I moved buildscript to the top since Checkstyle requested that.

Unexpected input values
I added this in 60_10_indices_filter.yml

Enable index filter options
I'm coding. It will take time. I hope you can review the items above first.

[lukas-vlcek]

Thanks for the update.

• Looking good at first glance.
• We can keep checkstyle plugin deactivated for now and we will tune the rules later. Adding the plugin now is nice.
• Regarding enabling index filter options I think we might be good to just make sure that use of non-existent or closed index will not make whole request to return error. Maybe we can initially go with hardcoded but relaxed index options like this:

indicesOptions = IndicesOptions.lenientExpandOpen();
// or indicesOptions = IndicesOptions.lenientExpand(); // ??
new IndicesStatsRequest().indicesOptions(indicesOptions).indices(...)

I am not sure how to deal with closed indices (i.e. lenientExpandOpen() vs lenientExpand()). Does the API return any useful metrics for them? (It would be great to have a test for closed indices as well.)

[tntruong723]

I'm done enabling index filter options. Could you please help review it?
I just saw your last comment today. My approach is a pretty big difference. :))

[lukas-vlcek]

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 PROMETHEUS_OPTIONS_DESCRIPTION_KEY property

What is the intention to expose the PROMETHEUS_OPTIONS_DESCRIPTION_KEY property? This translates to the prometheus.indices_filter.options_description key in the cluster settings.

While it is true that you can not update this property dynamically:

curl -s -H "Content-Type: application/json" -X PUT \
  localhost:9200/_cluster/settings\?pretty -d '{
    "transient": {
       "prometheus.indices_filter.options_description": "test"
    }
  }'

# Response:
{
  "error" : {
    "root_cause" : [
      {
        "type" : "illegal_argument_exception",
        "reason" : "transient setting [prometheus.indices_filter.options_description], not dynamically updateable"
      }
    ],
    "type" : "illegal_argument_exception",
    "reason" : "transient setting [prometheus.indices_filter.options_description], not dynamically updateable"
  },
  "status" : 400
}

It is still possible to change the value of it in the node configuration file.

# Add config line to opensearch.yaml file
cd <opensearch_node_home>
echo "prometheus.indices_filter.options_description: [ 'Call my number to get discount code!' ]" >> ./config/opensearch.yml
# Start the node
./bin/opensearch

# Now, when you get the cluster settings you see this:
curl -s -H "Content-Type: application/json" -X GET \
  localhost:9200/_cluster/settings\?include_defaults=true\&filter_path=defaults.prometheus\&pretty

#Response:
{
  "defaults" : {
    "prometheus" : {
      "cluster" : {
        "settings" : "true"
      },
      "indices" : "true",
      "nodes" : {
        "filter" : "_local"
      },
      "indices_filter" : {
        "options_description" : [
          "Call my number to get discount code!"
        ],
        "selected_indices" : "",
        "selected_option" : "STRICT_EXPAND_OPEN_FORBID_CLOSED"
      },
      "metric_name" : {
        "prefix" : "opensearch_"
      }
    }
  }
}

I think I understand what you tried to do here but I am not sure this is the best approach. While I like the idea of the system being able to give user the complete information I would say that in this case the plugin documentation is the better place at the moment.

In the end it should be the OpenSearch who shall be able to give user the list of all possible index filter options and this plugin should not substitute missing API for that.

Theoretically we can put the list of available options into an error message (exception trace) but that would be hard to parse anyway...

2) Missing STRICT_EXPAND_OPEN_HIDDEN_FORBID_CLOSED option?

According to the IndicesOptions.java file there are 12 index filter options in total:

1. STRICT_EXPAND_OPEN
2. STRICT_EXPAND_OPEN_HIDDEN
3. LENIENT_EXPAND_OPEN
4. LENIENT_EXPAND_OPEN_HIDDEN
5. LENIENT_EXPAND_OPEN_CLOSED
6. LENIENT_EXPAND_OPEN_CLOSED_HIDDEN
7. STRICT_EXPAND_OPEN_CLOSED
8. STRICT_EXPAND_OPEN_CLOSED_HIDDEN
9. STRICT_EXPAND_OPEN_FORBID_CLOSED
10. STRICT_EXPAND_OPEN_HIDDEN_FORBID_CLOSED
11. STRICT_EXPAND_OPEN_FORBID_CLOSED_IGNORE_THROTTLED
12. STRICT_SINGLE_INDEX_NO_EXPAND_FORBID_CLOSED

But the option called STRICT_EXPAND_OPEN_HIDDEN_FORBID_CLOSED does not seem to be exposed via a "getter method" but still it can be obtained directly through IndicesOptions.STRICT_EXPAND_OPEN_HIDDEN_FORBID_CLOSED.

I am not sure why it is like that (I think I will open a ticket about this in OpenSearch) but I can not think about any reason why we should not include this option as well. WDYT?

Other than that it is LGTM!
Again, thank you for your hard work on this.

Lukáš

[tntruong723]

Thank you for your comments.

1) About the PROMETHEUS_OPTIONS_DESCRIPTION_KEY property
Actually, I just want to give users the complete information so that users can choose a suitable option.
As I understood your comment, I should remove the PROMETHEUS_OPTIONS_DESCRIPTION_KEY property, then put the information in README.md
Let me do it. Please correct me if I misunderstood.

2) About the STRICT_EXPAND_OPEN_HIDDEN_FORBID_CLOSED option
I didn't see it before. You are very careful. There are many learnings for me from your comments. Thanks.
I think maybe they missed a getter method for this option :))
Anyway, let me add this option to the plugin.

[lukas-vlcek]

@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 prometheus.indices_filter.options_description in the node config file). So I think it would be better to just put it into the documentation. Maybe we can point users to IndicesOptions.java source file as well.

Thank you very much!

[tntruong723]

@lukas-vlcek I fixed your review comments. Sorry for the late update. Many things need to be done in my project recently.

[lukas-vlcek]

Looking good to me. Thank you very much for this contribution!