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

[rule based autotagging] Add Get Rule API Logic #17336

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ruai0511
Copy link
Contributor

Description

This PR introduces the get Rule API Logic.

An example API request is:

curl -XGET "localhost:9200/_wlm/rule" // get all rules
curl -XGET "localhost:9200/_wlm/rule/{_id}" // get single rule by id
curl -XGET "localhost:9200/_wlm/rule?index_pattern=a,b" // get all rules containing index_pattern as a or b 

And the return would be

{
   "rules":[
      {
         "_id":"LN1z8pQBW57iKqfUCG6A",
         "index_pattern":[
            "logs*",
            "events*"
         ],
         "query_group":"dev_query_group_id_2",
         "updated_at":"2025-02-11T00:40:12.671Z"
      },
      ...
   ]
}

Rule Schema PR:
#17238
RFC:
#16797

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

Copy link
Contributor

❌ Gradle check result for db57bcb: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@ruai0511 ruai0511 changed the title Add Get Rule API Logic [rule based autotagging] Add Get Rule API Logic Feb 12, 2025
Copy link
Contributor

❌ Gradle check result for f7bea2e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>
Copy link
Contributor

❌ Gradle check result for 4d2d843: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

* A request for get Rule
* @opensearch.experimental
*/
public class GetRuleRequest extends ClusterManagerNodeRequest<GetRuleRequest> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not be extending this class from ClusterManagerNodeRequest since this operation will be carried out on a data node.

Comment on lines +27 to +30
private final String _id;
private final Map<RuleAttribute, Set<String>> attributeFilters;

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add more details about the request and response behavior in the details of the PR ? For example what will be the structure of query payload and all. Can we confirm how other system indices support this behavior to keep the behavior uniform.

Copy link
Contributor Author

@ruai0511 ruai0511 Feb 14, 2025

Choose a reason for hiding this comment

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

System Indices like .opensearch_*, .monitoring, .tasks, .security, .ml, etc., generally support query parameters like filters, pagination (search_after), and sorting. But those are from endpoint that supports search operations (e.g., _search, _count, etc.), and will automatically handle those parameters. For us it's a bit different because we're implementing our own get api. At this point i think supporting search_after and size should be enough

* Response for the get API for Rule
* @opensearch.experimental
*/
public class GetRuleResponse extends ActionResponse implements ToXContent, ToXContentObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a sample response in the class level comment ?

Comment on lines +160 to +162
if (matchesFilters(currRule, attributeFilters)) {
return Map.entry(hit.getId(), currRule);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we should pass these filters to the request itself so that the filtering happens at the lucene layer and we get more accurate results back rather than all the Rules from the index ?

private void fetchAllRules(Map<RuleAttribute, Set<String>> attributeFilters, ActionListener<GetRuleResponse> listener) {
ThreadContext.StoredContext storedContext = client.threadPool().getThreadContext().stashContext();
client.prepareSearch(RULE_INDEX)
.setQuery(QueryBuilders.matchAllQuery())
Copy link
Contributor

Choose a reason for hiding this comment

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

This query should contain all the filters and query clauses, that will help us remove majority of the logic in this class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I modified the logic

* @opensearch.experimental
*/
public class RulePersistenceService {
public static final String RULE_INDEX = ".rule";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rename this to either .rules or .rule_index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to .rules because i think including the word 'index' in index name seems a bit redundant

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