[Backport 3.0] Monitoring: Accurately capture predict failures (#4525)

[pyek-bot]

Summary

• Backport of [Backport 3.3] Monitoring: Accurately capture predict failures #4555 to 3.0
• Introduces shouldTrackRemoteFailure(Exception e) method that skips tracking for user/configuration issues (4xx HTTP errors, IllegalArgumentException) while still tracking infrastructure/service issues (5xx errors, other exceptions)
• Fixes two call sites in runPredict that previously hard-coded false (never tracking remote failures)

Test plan

• Unit test testShouldTrackRemoteFailure validates all scenarios

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit de51c56)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to de51c56
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle wrapped exception causes The method doesn't handle wrapped exceptions where the root cause might be an IllegalArgumentException or OpenSearchStatusException. Consider checking the exception's cause chain to ensure nested exceptions are properly classified. plugin/src/main/java/org/opensearch/ml/task/MLPredictTaskRunner.java [608-624] boolean shouldTrackRemoteFailure(Exception e) { - // Don't track failures for user configuration issues - if (e instanceof IllegalArgumentException) { - return false; - } - - // Don't track any 4xx client errors (user/configuration issues) - if (e instanceof OpenSearchStatusException) { - RestStatus status = ((OpenSearchStatusException) e).status(); - if (status.getStatus() >= 400 && status.getStatus() < 500) { + // Check the exception and its causes + Throwable current = e; + while (current != null) { + // Don't track failures for user configuration issues + if (current instanceof IllegalArgumentException) { return false; } + + // Don't track any 4xx client errors (user/configuration issues) + if (current instanceof OpenSearchStatusException) { + RestStatus status = ((OpenSearchStatusException) current).status(); + if (status.getStatus() >= 400 && status.getStatus() < 500) { + return false; + } + } + current = current.getCause(); } // Track failures for infrastructure/service issues return true; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the current implementation doesn't check the exception cause chain. This could lead to misclassification of wrapped exceptions where the root cause is a user configuration error (4xx) but gets tracked as an infrastructure failure. The improved code properly traverses the exception chain to handle nested exceptions.	Medium

---

Previous suggestions

Suggestions up to commit de51c56

Category	Suggestion	Impact
General	Check exception cause chain The method doesn't handle wrapped exceptions where the root cause might be an IllegalArgumentException or OpenSearchStatusException. Consider checking the exception's cause chain to ensure user configuration errors are properly identified even when wrapped in other exception types. plugin/src/main/java/org/opensearch/ml/task/MLPredictTaskRunner.java [608-624] boolean shouldTrackRemoteFailure(Exception e) { // Don't track failures for user configuration issues - if (e instanceof IllegalArgumentException) { - return false; - } - - // Don't track any 4xx client errors (user/configuration issues) - if (e instanceof OpenSearchStatusException) { - RestStatus status = ((OpenSearchStatusException) e).status(); - if (status.getStatus() >= 400 && status.getStatus() < 500) { + Throwable current = e; + while (current != null) { + if (current instanceof IllegalArgumentException) { return false; } + + // Don't track any 4xx client errors (user/configuration issues) + if (current instanceof OpenSearchStatusException) { + RestStatus status = ((OpenSearchStatusException) current).status(); + if (status.getStatus() >= 400 && status.getStatus() < 500) { + return false; + } + } + current = current.getCause(); } // Track failures for infrastructure/service issues return true; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that wrapped exceptions might not be properly handled. Checking the cause chain ensures that user configuration errors (like IllegalArgumentException or 4xx OpenSearchStatusException) are detected even when wrapped, improving the robustness of error classification. However, this is a defensive improvement rather than fixing a critical bug.	Medium

[github-actions]

Persistent review updated to latest commit de51c56

[pyek-bot]

Spotless failing due to:

* What went wrong:
A problem occurred configuring root project 'opensearch-ml'.
> Could not resolve all artifacts for configuration 'classpath'.
   > Could not find org.opensearch.gradle:build-tools:3.0.0-SNAPSHOT.
     Searched in the following locations:
       - file:/home/runner/.m2/repository/org/opensearch/gradle/build-tools/3.0.0-SNAPSHOT/maven-metadata.xml