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 alerting tools IT; fix missing system index bug of SearchMonitorsTool #135

Merged
merged 9 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ dependencies {
zipArchive group: 'org.opensearch.plugin', name:'opensearch-sql-plugin', version: "${version}"
zipArchive group: 'org.opensearch.plugin', name:'opensearch-knn', version: "${version}"
zipArchive group: 'org.opensearch.plugin', name:'neural-search', version: "${version}"
zipArchive group: 'org.opensearch.plugin', name:'alerting', version: "${version}"

// Test dependencies
testImplementation "org.opensearch.test:framework:${opensearch_version}"
Expand Down Expand Up @@ -388,4 +389,4 @@ task updateVersion {
// Include the required files that needs to be updated with new Version
ant.replaceregexp(file:'build.gradle', match: '"opensearch.version", "\\d.*"', replace: '"opensearch.version", "' + newVersion.tokenize('-')[0] + '-SNAPSHOT"', flags:'g', byline:true)
}
}
}
83 changes: 55 additions & 28 deletions src/main/java/org/opensearch/agent/tools/SearchMonitorsTool.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@
package org.opensearch.agent.tools;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import org.apache.commons.lang3.StringUtils;
import org.apache.lucene.search.join.ScoreMode;
Expand All @@ -20,6 +23,7 @@
import org.opensearch.commons.alerting.action.GetMonitorResponse;
import org.opensearch.commons.alerting.action.SearchMonitorRequest;
import org.opensearch.commons.alerting.model.Monitor;
import org.opensearch.commons.alerting.model.ScheduledJob;
import org.opensearch.core.action.ActionListener;
import org.opensearch.index.query.BoolQueryBuilder;
import org.opensearch.index.query.ExistsQueryBuilder;
Expand Down Expand Up @@ -104,22 +108,17 @@
if (monitorId != null) {
GetMonitorRequest getMonitorRequest = new GetMonitorRequest(monitorId, 1L, RestRequest.Method.GET, null);
ActionListener<GetMonitorResponse> getMonitorListener = ActionListener.<GetMonitorResponse>wrap(response -> {
StringBuilder sb = new StringBuilder();
Monitor monitor = response.getMonitor();
if (monitor != null) {
sb.append("Monitors=[");
sb.append("{");
sb.append("id=").append(monitor.getId()).append(",");
sb.append("name=").append(monitor.getName());
sb.append("}]");
sb.append("TotalMonitors=1");
processGetMonitorHit(monitor, listener);
}, e -> {
// System index isn't initialized by default, so ignore such errors. Alerting plugin does not return the
// standard IndexNotFoundException so we parse the message instead
if (e.getMessage().contains("Configured indices are not found")) {
processGetMonitorHit(null, listener);

Check warning on line 117 in src/main/java/org/opensearch/agent/tools/SearchMonitorsTool.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/agent/tools/SearchMonitorsTool.java#L117

Added line #L117 was not covered by tests
} else {
sb.append("Monitors=[]TotalMonitors=0");
log.error("Failed to get monitor.", e);
listener.onFailure(e);

Check warning on line 120 in src/main/java/org/opensearch/agent/tools/SearchMonitorsTool.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/agent/tools/SearchMonitorsTool.java#L119-L120

Added lines #L119 - L120 were not covered by tests
}
listener.onResponse((T) sb.toString());
}, e -> {
log.error("Failed to search monitors.", e);
listener.onFailure(e);
});
AlertingPluginInterface.INSTANCE.getMonitor((NodeClient) client, getMonitorRequest, getMonitorListener);
} else {
Expand Down Expand Up @@ -167,24 +166,23 @@
.from(startIndex)
.sort(sortString, sortOrder);

SearchMonitorRequest searchMonitorRequest = new SearchMonitorRequest(new SearchRequest().source(searchSourceBuilder));
SearchRequest searchRequest = new SearchRequest().source(searchSourceBuilder).indices(ScheduledJob.SCHEDULED_JOBS_INDEX);
SearchMonitorRequest searchMonitorRequest = new SearchMonitorRequest(searchRequest);

ActionListener<SearchResponse> searchMonitorListener = ActionListener.<SearchResponse>wrap(response -> {
StringBuilder sb = new StringBuilder();
SearchHit[] hits = response.getHits().getHits();
sb.append("Monitors=[");
for (SearchHit hit : hits) {
sb.append("{");
sb.append("id=").append(hit.getId()).append(",");
sb.append("name=").append(hit.getSourceAsMap().get("name"));
sb.append("}");
}
sb.append("]");
sb.append("TotalMonitors=").append(response.getHits().getTotalHits().value);
listener.onResponse((T) sb.toString());
List<SearchHit> hits = Arrays.asList(response.getHits().getHits());
Map<String, SearchHit> hitsAsMap = hits.stream().collect(Collectors.toMap(SearchHit::getId, hit -> hit));
processHits(hitsAsMap, listener);

}, e -> {
log.error("Failed to search monitors.", e);
listener.onFailure(e);
// System index isn't initialized by default, so ignore such errors. Alerting plugin does not return the
// standard IndexNotFoundException so we parse the message instead
if (e.getMessage().contains("Configured indices are not found")) {
processHits(Collections.emptyMap(), listener);

Check warning on line 181 in src/main/java/org/opensearch/agent/tools/SearchMonitorsTool.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/agent/tools/SearchMonitorsTool.java#L181

Added line #L181 was not covered by tests
} else {
log.error("Failed to search monitors.", e);
listener.onFailure(e);

Check warning on line 184 in src/main/java/org/opensearch/agent/tools/SearchMonitorsTool.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/agent/tools/SearchMonitorsTool.java#L183-L184

Added lines #L183 - L184 were not covered by tests
}
});
AlertingPluginInterface.INSTANCE.searchMonitors((NodeClient) client, searchMonitorRequest, searchMonitorListener);
}
Expand All @@ -200,6 +198,35 @@
return TYPE;
}

private <T> void processHits(Map<String, SearchHit> hitsAsMap, ActionListener<T> listener) {
StringBuilder sb = new StringBuilder();
sb.append("Monitors=[");
for (SearchHit hit : hitsAsMap.values()) {
sb.append("{");
sb.append("id=").append(hit.getId()).append(",");
sb.append("name=").append(hit.getSourceAsMap().get("name"));
sb.append("}");
}
sb.append("]");
sb.append("TotalMonitors=").append(hitsAsMap.size());
listener.onResponse((T) sb.toString());
}

private <T> void processGetMonitorHit(Monitor monitor, ActionListener<T> listener) {
StringBuilder sb = new StringBuilder();
if (monitor != null) {
sb.append("Monitors=[");
sb.append("{");
sb.append("id=").append(monitor.getId()).append(",");
sb.append("name=").append(monitor.getName());
sb.append("}]");
sb.append("TotalMonitors=1");
} else {
sb.append("Monitors=[]TotalMonitors=0");
}
listener.onResponse((T) sb.toString());
}

/**
* Factory for the {@link SearchMonitorsTool}
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ public static enum DetectorStateString {
Initializing
}

// System indices constants are not cleanly exposed from the AD plugin, so we persist our
// own constant here.
// System indices constants are not cleanly exposed from the AD & Alerting plugins, so we persist our
// own constants here.
public static final String AD_RESULTS_INDEX_PATTERN = ".opendistro-anomaly-results*";
public static final String AD_DETECTORS_INDEX = ".opendistro-anomaly-detectors";

public static final String ALERTING_CONFIG_INDEX = ".opendistro-alerting-config";
}
57 changes: 57 additions & 0 deletions src/test/java/org/opensearch/integTest/SearchAlertsToolIT.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.integTest;

import java.nio.file.Files;
import java.nio.file.Path;

import org.junit.After;
import org.junit.Before;

import lombok.SneakyThrows;

public class SearchAlertsToolIT extends BaseAgentToolsIT {
private String registerAgentRequestBody;
private static final String monitorId = "foo-id";
private static final String monitorName = "foo-name";

@Before
@SneakyThrows
public void setUp() {
super.setUp();
registerAgentRequestBody = Files
.readString(
Path
.of(
this
.getClass()
.getClassLoader()
.getResource("org/opensearch/agent/tools/register_flow_agent_of_search_alerts_tool_request_body.json")
.toURI()
)
);
}

@After
@SneakyThrows
public void tearDown() {
super.tearDown();
deleteExternalIndices();
deleteSystemIndices();
}

@SneakyThrows
public void testSearchAlertsToolInFlowAgent_withNoSystemIndex() {
deleteSystemIndices();
String agentId = createAgent(registerAgentRequestBody);
String agentInput = "{\"parameters\":{\"monitorId\": \"" + monitorId + "\"}}";
String result = executeAgent(agentId, agentInput);
assertEquals("Alerts=[]TotalAlerts=0", result);
}

// TODO: Add IT to test against sample alerts data
// https://github.com/opensearch-project/skills/issues/136
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.integTest;

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;

import org.junit.After;
import org.junit.Before;
import org.opensearch.agent.tools.utils.ToolConstants;

import lombok.SneakyThrows;

public class SearchMonitorsToolIT extends BaseAgentToolsIT {
private String registerAgentRequestBody;
private static final String monitorId = "foo-id";
private static final String monitorName = "foo-name";

@Before
@SneakyThrows
public void setUp() {
super.setUp();
registerAgentRequestBody = Files
.readString(
Path
.of(
this
.getClass()
.getClassLoader()
.getResource("org/opensearch/agent/tools/register_flow_agent_of_search_monitors_tool_request_body.json")
.toURI()
)
);
createMonitorsSystemIndex(monitorId, monitorName);
}

@After
@SneakyThrows
public void tearDown() {
super.tearDown();
deleteExternalIndices();
deleteSystemIndices();
}

@SneakyThrows
public void testSearchMonitorsToolInFlowAgent_withNoSystemIndex() {
deleteSystemIndices();
String agentId = createAgent(registerAgentRequestBody);
String agentInput = "{\"parameters\":{\"monitorName\": \"" + monitorName + "\"}}";
String result = executeAgent(agentId, agentInput);
assertEquals("Monitors=[]TotalMonitors=0", result);
}

// TODO: Add IT to test against sample monitor data

@SneakyThrows
private void createMonitorsSystemIndex(String monitorId, String monitorName) {
Copy link
Member

Choose a reason for hiding this comment

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

We are trying to create the system index for alerting. The best way to do this is to create a dummy monitor, so that the schema mapping of the index is setup correctly and would be better to test for an integration test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed - this was an interim test fn that is actually currently not used. I'll remove and leave the TODO, as this will be grouped with the added tests in the followup issue #136. Thanks for the suggestion, I will likely create dummy monitors and alerts for testing the populated results of the search monitors and search alerts tools respectively.

createIndexWithConfiguration(
ToolConstants.ALERTING_CONFIG_INDEX,
"{\n"
+ " \"mappings\": {\n"
+ " \"properties\": {\n"
+ " \"name\": {\n"
+ " \"type\": \"text\",\n"
+ " \"fields\": { \"keyword\": { \"type\": \"keyword\", \"ignore_above\": 256 }}"
+ " }\n"
+ " }\n"
+ " }\n"
+ "}"
);
addDocToIndex(ToolConstants.ALERTING_CONFIG_INDEX, monitorId, List.of("name"), List.of(monitorName));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "Test_Search_Alerts_Agent",
"type": "flow",
"tools": [
{
"type": "SearchAlertsTool",
"description": "Use this tool to search alerts."
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "Test_Search_Monitors_Agent",
"type": "flow",
"tools": [
{
"type": "SearchMonitorsTool",
"description": "Use this tool to search alerting monitors."
}
]
}
Loading