[Sandbox] Track planning failures in analytics stats API

[finnegancarroll]

Adds a planning_failures counter to the _plugins/_analytics/stats endpoint. Increments when an exception escapes the planning phase before execution begins.

Changes

• AnalyticsStatsCollector: new LongAdder + recordPlanningFailure()
• AnalyticsStats.Queries: new planningFailures field with serialization
• DefaultPlanExecutor.doExecute: call recordPlanningFailure() in catch blocks
• Unit tests updated

Response shape

{"analytics": {"queries": {"planning_failures": 3, ...}}}

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 046702e)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incomplete failure tracking recordPlanningFailure() is only called from the two synchronous catch blocks in doExecute. Planning failures that surface asynchronously via convertingListener.onFailure (e.g., failures propagated from within executeInternal's callbacks, or the result.failure() path just above) are not counted. If planning is largely asynchronous, the counter may significantly under-report actual planning-phase failures, making the metric misleading. Confirm whether the intent is to count only failures thrown synchronously from the planning entry point, or all pre-execution failures. } catch (Exception e) { statsCollector.recordPlanningFailure(); convertingListener.onFailure(e); } catch (AssertionError e) { statsCollector.recordPlanningFailure(); convertingListener.onFailure( new IllegalStateException("Analytics-engine executor rejected the plan: " + e.getMessage(), e) );

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 046702e

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Also count asynchronous planning failures The planning-failure counter is only incremented for synchronous exceptions thrown from executeInternal. If planning fails asynchronously and is delivered via convertingListener::onFailure (the second argument of the inner listener), recordPlanningFailure() is never called, undercounting real planning failures. Route the async failure path through a wrapper that increments the counter before delegating. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [459-467] - } catch (Exception e) { -+ statsCollector.recordPlanningFailure(); + }, e -> { + statsCollector.recordPlanningFailure(); convertingListener.onFailure(e); - } catch (AssertionError e) { -+ statsCollector.recordPlanningFailure(); - convertingListener.onFailure( - new IllegalStateException("Analytics-engine executor rejected the plan: " + e.getMessage(), e) - ); - } + }) + ); + } catch (Exception e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure(e); + } catch (AssertionError e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure( + new IllegalStateException("Analytics-engine executor rejected the plan: " + e.getMessage(), e) + ); + } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern that async planning failures delivered through the inner listener's onFailure won't be counted, potentially undercounting failures. However, without full context of what convertingListener::onFailure represents (it may include execution-phase failures too, not just planning), the fix could over-count. Still a reasonable observation about completeness of the metric.	Low

---

Previous suggestions

Suggestions up to commit 385635e

Category	Suggestion	Impact
General	Also count async planning failures The planning failure counter only increments for synchronous exceptions escaping the try block, but planning failures delivered asynchronously via the listener's onFailure path (the convertingListener::onFailure argument on the inner listener) are not counted. Consider also incrementing the counter in the async failure callback so that all planning-phase failures are captured, not just the synchronous subset. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [459-467] +} catch (Exception e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure(e); +} catch (AssertionError e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure( + new IllegalStateException("Analytics-engine executor rejected the plan: " + e.getMessage(), e) + ); +} - Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern that async planning failures delivered via convertingListener::onFailure are not counted, which could lead to undercounted metrics. However, the improved_code is identical to existing_code and doesn't actually demonstrate the fix, and without deeper context it's unclear which async failures represent planning vs. execution failures.	Low

Suggestions up to commit 6d5b082

Category	Suggestion	Impact
General	Also count async planning failures The planning-failure counter is only incremented for synchronous exceptions thrown from executeInternal. If planning fails asynchronously and the failure is delivered via convertingListener.onFailure (e.g. the inner listener's failure path a few lines above where result.failure() is propagated), the counter will not reflect it. Consider also incrementing recordPlanningFailure() on the async failure branch(es) so the metric captures all planning-phase failures, not just synchronous ones. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [459-467] +} catch (Exception e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure(e); +} catch (AssertionError e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure( + new IllegalStateException("Analytics-engine executor rejected the plan: " + e.getMessage(), e) + ); +} - Suggestion importance[1-10]: 5 __ Why: The observation is reasonable — the counter only captures synchronous exceptions from executeInternal, potentially missing async planning failures delivered via convertingListener.onFailure. However, the suggestion is speculative without full visibility of the async path, and the improved_code is identical to existing_code, offering no concrete fix.	Low

Suggestions up to commit 802b90c

Category	Suggestion	Impact
General	Also count async planning failures Planning failures are only counted for synchronous exceptions thrown from executeInternal. If planning fails asynchronously and surfaces via the inner listener's onFailure (the convertingListener::onFailure branch), the failure will not be recorded. Consider also incrementing recordPlanningFailure() on the async failure path to avoid undercounting, or clearly document that this metric only covers synchronous planning failures. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [459-467] +} catch (Exception e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure(e); +} catch (AssertionError e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure( + new IllegalStateException("Analytics-engine executor rejected the plan: " + e.getMessage(), e) + ); +} - Suggestion importance[1-10]: 6 __ Why: Valid observation that only synchronous planning failures are counted; async failures via the listener's onFailure branch won't be recorded. However, the suggestion's improved_code is identical to the existing_code, only recommending consideration.	Low
	Broaden accepted error status codes fail(...) throws AssertionError, which is not caught by the ResponseException catch block, so if the request unexpectedly succeeds the assertion will still propagate — but more importantly, if the failure surfaces as a 5xx (e.g., wrapped as ISE) the test will fail even though a planning failure was recorded. Consider broadening the accepted status range or asserting on the stats delta only, which is the actual behavior under test. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/AnalyticsStatsApiIT.java [164-170] try { client().performRequest(request); - fail("Expected a 400 for unsupported function"); + fail("Expected an error response for unsupported function"); } catch (org.opensearch.client.ResponseException e) { int status = e.getResponse().getStatusLine().getStatusCode(); - assertTrue("Expected 4xx for planning failure, got " + status, status >= 400 && status < 500); + assertTrue("Expected error status for planning failure, got " + status, status >= 400); } Suggestion importance[1-10]: 4 __ Why: Minor test robustness improvement to accept 5xx status codes for planning failures, since the test's real assertion is on the stats delta. Impact is limited to test stability.	Low

Suggestions up to commit e50e53d

Category	Suggestion	Impact
General	Capture async planning failures too The planning failure is only recorded when an exception escapes the try block synchronously, but failures propagated through the listener's onFailure callback (e.g., from the async planning path or convertingListener::onFailure) are not counted. Consider also recording the failure in the listener's failure callback to ensure all planning failures are captured, or clearly document that this counter reflects only synchronous planning rejections. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [459-467] +} catch (Exception e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure(e); +} catch (AssertionError e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure( + new IllegalStateException("Analytics-engine executor rejected the plan: " + e.getMessage(), e) + ); +} - Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern that async planning failures propagated through the listener's onFailure callback are not counted, potentially undercounting failures. However, existing_code and improved_code are identical, so it's more of an observation than a concrete fix.	Low

Suggestions up to commit 0cf78c9

Category	Suggestion	Impact
General	Also count async planning failures The planning failure counter is only incremented when the synchronous try block throws, but not when the async listener path (result.failure() branch calling convertingListener.onFailure) reports a planning-phase failure. Consider also recording a planning failure in that branch (or wherever planning is known to have failed asynchronously) so the metric reflects all planning failures, not just synchronous ones. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [459-467] +} catch (Exception e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure(e); +} catch (AssertionError e) { + statsCollector.recordPlanningFailure(); + convertingListener.onFailure( + new IllegalStateException("Analytics-engine executor rejected the plan: " + e.getMessage(), e) + ); +} - Suggestion importance[1-10]: 5 __ Why: The suggestion raises a reasonable concern that async planning failures via result.failure() may not be counted, but without full visibility into the async path and what constitutes a planning failure there, it's speculative. Also, existing_code equals improved_code, making the suggestion advisory only.	Low

[github-actions]

❌ Gradle check result for 0075fa0: 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?

[github-actions]

Persistent review updated to latest commit 0cf78c9

[github-actions]

❌ Gradle check result for 0cf78c9: 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?

[github-actions]

Persistent review updated to latest commit e50e53d

[github-actions]

Persistent review updated to latest commit 802b90c

[github-actions]

❌ Gradle check result for 802b90c: 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?

[github-actions]

Persistent review updated to latest commit 6d5b082

[github-actions]

✅ Gradle check result for 6d5b082: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.52%. Comparing base (25c32e5) to head (6d5b082).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22667      +/-   ##
============================================
- Coverage     71.52%   71.52%   -0.01%     
+ Complexity    77023    76999      -24     
============================================
  Files          6156     6156              
  Lines        358422   358422              
  Branches      52245    52245              
============================================
- Hits         256351   256349       -2     
- Misses        81694    81702       +8     
+ Partials      20377    20371       -6     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit 385635e

[github-actions]

❌ Gradle check result for 385635e: 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?

[github-actions]

Persistent review updated to latest commit 046702e

[github-actions]

❌ Gradle check result for 046702e: 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?