Add basic search detectors tool; pull plugin deps in gradle run #39

ohltyler · 2023-12-19T19:21:25Z

Description

This PR adds 2 things:

Adds basic implementation for part of the search detectors tool. Remaining functionality will be added when the profile API is onboarded in the AD client
Adds dependencies on AD and job scheduler
Improves ./gradlew run to run an integ test cluster with ML commons, SQL, AD, and job scheduler plugins for faster iterative testing

Confirmed all build / test / run cmds work successfully on 2.11. The only difference for main is bumping to 3.0. Given the dependency list I'm not enforcing 3.0 builds to pass.

Additionally, I will manually open a backport to 2.x with the correct version

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Tyler Ohlsen <[email protected]>

dbwiddis

A few nits.

dbwiddis · 2023-12-19T23:01:26Z

build.gradle


 configurations {
    zipArchive
    all {
        resolutionStrategy {
            force "org.mockito:mockito-core:5.8.0"
-            force "com.google.guava:guava:32.1.3-jre" // CVE for 31.1
+            force "com.google.guava:guava:32.1.2-jre" // CVE for 31.1


Can we add a comment why we are not using the latest version? Mend will generate an auto-bump for this (if it hasn't already) and would like to include a reason when closing it to prevent future bumps...

Thanks for the catch. Reverted this as it's a leftover change from my inter-plugin dependency issues.

src/main/java/org/opensearch/agent/tools/SearchAnomalyDetectorsTool.java

src/test/java/org/opensearch/agent/tools/SearchAnomalyDetectorsToolTests.java

zane-neo · 2023-12-20T02:22:41Z

Build is failing, seems like missing dependency.

ohltyler · 2023-12-20T16:52:50Z

Build is failing, seems like missing dependency.

@zane-neo See comment in description:

Confirmed all build / test / run cmds work successfully on 2.11. The only difference for main is bumping to 3.0. Given the dependency list I'm not enforcing 3.0 builds to pass.

It is unsustainable to maintain passing builds on main since most plugin builds fail due to constant breaking changes on main/3.0. We can ensure passing builds on 2.x.

Signed-off-by: Tyler Ohlsen <[email protected]>

dbwiddis

LGTM with comments

build.gradle

src/main/java/org/opensearch/agent/tools/SearchAnomalyDetectorsTool.java

Signed-off-by: Tyler Ohlsen <[email protected]>

dbwiddis

LGTM with previous comments noted. However, not sure we should merge something that doesn't compile and breaks main. That will kill all testing of future PRs.

LGTM but dismissing approval so this doesn't get merged until dependency fixed.

Signed-off-by: Tyler Ohlsen <[email protected]>

codecov · 2023-12-21T21:20:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@62c9754). Click here to learn what that means.

❗ Current head 27c638f differs from pull request most recent head 00f35b0. Consider uploading reports for the commit 00f35b0 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #39   +/-   ##
=======================================
  Coverage        ?   21.21%           
  Complexity      ?        6           
=======================================
  Files           ?        3           
  Lines           ?      231           
  Branches        ?       31           
=======================================
  Hits            ?       49           
  Misses          ?      167           
  Partials        ?       15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ohltyler · 2023-12-21T21:21:58Z

LGTM with previous comments noted. However, not sure we should merge something that doesn't compile and breaks main. That will kill all testing of future PRs.

I've root caused this failure - the plugin name change has changed the underlying jar path in the zip, hence not adding it to the classpath. I also got AD to build and upload to sonatype so the snapshot is now successfully pulled in CI.

dbwiddis · 2023-12-21T23:05:08Z

the plugin name change

Ah, yes! Should have remembered that.

zane-neo · 2023-12-22T01:33:50Z

src/main/java/org/opensearch/agent/tools/SearchAnomalyDetectorsTool.java

+        final Boolean highCardinality = parameters.containsKey("highCardinality")
+            ? Boolean.parseBoolean(parameters.get("highCardinality"))
+            : null;
+        final Long lastUpdateTime = parameters.containsKey("lastUpdateTime") ? Long.parseLong(parameters.get("lastUpdateTime")) : null;


Long.parseLong might throw NumberFormatException if the input parameter is null or invalid number format, suggestion is to use StringUtils.isNumeric() to check if the input is a number.

Thanks! I will update.

zane-neo · 2023-12-22T01:37:11Z

src/main/java/org/opensearch/agent/tools/SearchAnomalyDetectorsTool.java

+        SearchRequest searchDetectorRequest = new SearchRequest().source(searchSourceBuilder);
+
+        if (running != null || disabled != null || failed != null) {
+            // TODO: add a listener to trigger when the first response is received, to trigger the profile API call


Is this missed to address or will be addressed in future?

Will be addressed in the future. I'll create a follow up issue alongside some other improvements soon.

Signed-off-by: Tyler Ohlsen <[email protected]>

ohltyler added 6 commits December 19, 2023 00:04

Add initial search AD tool; add AD dep

5b5cd6b

Signed-off-by: Tyler Ohlsen <[email protected]>

Remove dup variable

1a399fd

Signed-off-by: Tyler Ohlsen <[email protected]>

Add initial search AD tool with basic tests

10cbd38

Signed-off-by: Tyler Ohlsen <[email protected]>

Build/test working; run errors with jarhell

01d351c

Signed-off-by: Tyler Ohlsen <[email protected]>

Clean up build.gradle

75d4837

Signed-off-by: Tyler Ohlsen <[email protected]>

Revert to 3.0

7ab84c6

Signed-off-by: Tyler Ohlsen <[email protected]>

ohltyler requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, xinyual, amitgalitz, jackiehanyang, owaiskazi19, joshpalis, dbwiddis, kaituo, lezzago, eirsep, sbcd90, zhichao-aws and joshuali925 as code owners December 19, 2023 19:21

dbwiddis reviewed Dec 19, 2023

View reviewed changes

Address comments

8c37fba

Signed-off-by: Tyler Ohlsen <[email protected]>

dbwiddis reviewed Dec 21, 2023

View reviewed changes

build.gradle Show resolved Hide resolved

build.gradle Outdated Show resolved Hide resolved

build.gradle Show resolved Hide resolved

src/main/java/org/opensearch/agent/tools/SearchAnomalyDetectorsTool.java Show resolved Hide resolved

Revert guava dep change

eb9f128

Signed-off-by: Tyler Ohlsen <[email protected]>

dbwiddis previously approved these changes Dec 21, 2023

View reviewed changes

dbwiddis self-requested a review December 21, 2023 19:32

Change to time-series-analytics for plugin dep

27c638f

Signed-off-by: Tyler Ohlsen <[email protected]>

dbwiddis approved these changes Dec 21, 2023

View reviewed changes

zane-neo reviewed Dec 22, 2023

View reviewed changes

Add null/invalid check on lastUpdateTime

00f35b0

Signed-off-by: Tyler Ohlsen <[email protected]>

zane-neo approved these changes Dec 22, 2023

View reviewed changes

zane-neo merged commit 9c08646 into opensearch-project:main Dec 22, 2023
3 of 11 checks passed

ohltyler mentioned this pull request Dec 22, 2023

[Backport 2.x] Add basic search detectors tool; pull plugin deps in gradle run #50

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic search detectors tool; pull plugin deps in gradle run #39

Add basic search detectors tool; pull plugin deps in gradle run #39

ohltyler commented Dec 19, 2023 •

edited

Loading

dbwiddis left a comment

dbwiddis Dec 19, 2023

ohltyler Dec 20, 2023

zane-neo commented Dec 20, 2023

ohltyler commented Dec 20, 2023 •

edited

Loading

dbwiddis left a comment

dbwiddis left a comment

codecov bot commented Dec 21, 2023 •

edited

Loading

ohltyler commented Dec 21, 2023

dbwiddis commented Dec 21, 2023

zane-neo Dec 22, 2023

ohltyler Dec 22, 2023

zane-neo Dec 22, 2023

ohltyler Dec 22, 2023

Add basic search detectors tool; pull plugin deps in gradle run #39

Add basic search detectors tool; pull plugin deps in gradle run #39

Conversation

ohltyler commented Dec 19, 2023 • edited Loading

Description

Issues Resolved

Check List

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis Dec 19, 2023

Choose a reason for hiding this comment

ohltyler Dec 20, 2023

Choose a reason for hiding this comment

zane-neo commented Dec 20, 2023

ohltyler commented Dec 20, 2023 • edited Loading

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 21, 2023 • edited Loading

Codecov Report

ohltyler commented Dec 21, 2023

dbwiddis commented Dec 21, 2023

zane-neo Dec 22, 2023

Choose a reason for hiding this comment

ohltyler Dec 22, 2023

Choose a reason for hiding this comment

zane-neo Dec 22, 2023

Choose a reason for hiding this comment

ohltyler Dec 22, 2023

Choose a reason for hiding this comment

ohltyler commented Dec 19, 2023 •

edited

Loading

ohltyler commented Dec 20, 2023 •

edited

Loading

codecov bot commented Dec 21, 2023 •

edited

Loading