[Feature] Integrate SQL/PPL with query-insights plugin

[KishoreKicha14]

Description

Propagate SQL/PPL query metadata to OpenSearch's query-insights plugin so that DSL queries generated by the SQL engine are identifiable and traceable back to their originating SQL/PPL statement.

Changes:

• Add thread context headers (x-query-source, x-original-query, x-query-execution-id, x-query-phases) set before DSL execution
• Register headers via getTaskHeaders() so TaskManager copies them into SearchTask for query-insights to read
• Add QueryPhaseTracker to instrument parse/analyze/plan phases with wall-clock time, CPU time, and memory allocation per phase
• Execution ID links multiple DSL queries from a single SQL/PPL execution (e.g., JOINs, pagination)

Related Issues

Resolves #5677

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

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.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit c50c128.

Path	Line	Severity	Description
legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java	161	low	Raw SQL query text is stored in 'x-original-query' header and propagated cluster-wide via getTaskHeaders. Queries containing sensitive literal values (e.g. password predicates, PII in WHERE clauses) would be distributed to all cluster nodes and made visible to the query-insights plugin. This is a deliberate design choice but widens the exposure surface for sensitive query content beyond the originating node.
common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java	37	low	Use of private JDK API com.sun.management.ThreadMXBean for per-thread memory allocation tracking. This is a non-public API with no stability guarantees, though its use here is confined to best-effort observability and is guarded with a null check. Not malicious, but worth noting as it bypasses standard API boundaries.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 0 | Low: 2

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit c50c128)

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 Possible Issue Unconditional putHeader calls will throw IllegalArgumentException if the header already exists. OpenSearch's ThreadContext.putHeader throws when a key is already present. If this handler runs more than once per request context (e.g., retries, plugin chaining), the second call fails. The PPL path correctly guards each putHeader with 'if (threadContext.getHeader(...) == null)'. The SQL path should follow the same pattern. client .threadPool() .getThreadContext() .putHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER, "sql"); client .threadPool() .getThreadContext() .putHeader( QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER, java.util.UUID.randomUUID().toString()); String queryText = sqlRequest.getSql(); if (queryText != null) { if (queryText.length() > QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH) { queryText = queryText.substring(0, QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH); } client .threadPool() .getThreadContext() .putHeader(QuerySourceHeaders.ORIGINAL_QUERY_HEADER, queryText); } Deprecated API Thread.currentThread().getId() is deprecated since Java 19 and replaced by threadId(). The project targets Java 21, so this will produce deprecation warnings. Replace with Thread.currentThread().threadId() on lines 134 and 148. ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) : 0; } public void endCurrentPhase() { if (activePhase != null) { long elapsed = System.nanoTime() - activeStart; phases.merge(activePhase, elapsed, Long::sum); if (CPU_SUPPORTED) { long cpuElapsed = THREAD_MX.getCurrentThreadCpuTime() - activeCpuStart; cpuPhases.merge(activePhase, cpuElapsed, Long::sum); } if (SUN_THREAD_MX != null) { long memElapsed = SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) - activeMemStart; Resource Leak If executeWithCalcite throws an exception after QueryPhaseTracker.startOrRestore() but before writePhaseHeader() or clear(), the ThreadLocal and Log4j ThreadContext key '_sql_phase_tracker' are never cleaned. On pooled threads, this leaks state into subsequent requests. Add a finally block that calls QueryPhaseTracker.clear() to ensure cleanup on all exit paths. QueryPhaseTracker tracker = QueryPhaseTracker.startOrRestore(); tracker.beginPhase("analyze"); ProfileContext profileContext = QueryProfiling.activate(QueryContext.isProfileEnabled()); ProfileMetric analyzeMetric = profileContext.getOrCreateMetric(MetricName.ANALYZE); long analyzeStart = System.nanoTime(); CalciteClassLoaderHelper.withCalciteClassLoader( () -> { CalcitePlanContext context = CalcitePlanContext.create( buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType); context.setHighlightConfig(highlightConfig); // Wrap analyze with ANALYZING stage tracking RelNode relNode = StageErrorHandler.executeStage( QueryProcessingStage.ANALYZING, () -> analyze(plan, context), "while preparing and validating the query plan"); tracker.beginPhase("plan"); // Wrap plan conversion with PLAN_CONVERSION stage tracking RelNode calcitePlan = StageErrorHandler.executeStage( QueryProcessingStage.PLAN_CONVERSION, () -> withCheckedArithmetic( convertToCalcitePlan(relNode, context), context), "while converting the query to an executable plan"); analyzeMetric.set(System.nanoTime() - analyzeStart); tracker.endCurrentPhase(); tracker.endAll(); executeCalcitePlan(calcitePlan, context, listener, analyzeMetric, analyzeStart); }, QueryService.class); } catch (Throwable t) { Incomplete Cleanup In executeWithLegacy, the tracker begins the 'plan' phase but endAll() only fires inside executePlan if current() is non-null. If plan(analyzed) throws before reaching executePlan, phases remain dangling in the ThreadLocal and Log4j context. Wrap the entire try block in a finally that calls QueryPhaseTracker.clear(). QueryPhaseTracker tracker = QueryPhaseTracker.startOrRestore(); tracker.beginPhase("analyze"); LogicalPlan analyzed = analyze(plan, queryType); tracker.beginPhase("plan"); executePlan(analyzed, PlanContext.emptyPlanContext(), listener); } catch (Exception e) { if (calciteFailure.isPresent()) { // This happens if Calcite fell back to V2 due to some issue, and then V2 also failed. // https://github.com/opensearch-project/sql/issues/5060 propagateCalciteError(calciteFailure.get(), listener); } else { listener.onFailure(e); } } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to c50c128

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent duplicate header exception Guard each putHeader call with a null-check on getHeader to prevent IllegalArgumentException if the header already exists. OpenSearch's ThreadContext.putHeader throws when a key is already present, which can occur in retry or plugin-chaining scenarios. legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java [158-166] -client - .threadPool() - .getThreadContext() - .putHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER, "sql"); -client - .threadPool() - .getThreadContext() - .putHeader( - QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER, java.util.UUID.randomUUID().toString()); +if (client.threadPool().getThreadContext().getHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER) == null) { + client.threadPool().getThreadContext().putHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER, "sql"); +} +if (client.threadPool().getThreadContext().getHeader(QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER) == null) { + client.threadPool().getThreadContext().putHeader( + QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER, java.util.UUID.randomUUID().toString()); +} Suggestion importance[1-10]: 8 __ Why: This addresses a real issue where putHeader throws IllegalArgumentException if the header already exists. The PPL path (lines 188-202 in TransportPPLQueryAction.java) correctly guards with null-checks, but the SQL path does not. This inconsistency could cause failures in retry or plugin-chaining scenarios, making it a significant correctness issue.	Medium
General	Replace deprecated Thread.getId() call Replace deprecated Thread.currentThread().getId() with Thread.currentThread().threadId(). The getId() method has been deprecated since Java 19, and the project targets Java 21. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [132-135] activeMemStart = SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) + ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) : 0; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies the use of deprecated Thread.currentThread().getId() (deprecated since Java 19) and proposes replacing it with threadId(). Since the project targets Java 21, this is a valid modernization that removes deprecation warnings. However, it's not a critical bug—just a code quality improvement.	Medium
	Prevent ThreadLocal leak on exceptions Add a finally block that calls QueryPhaseTracker.clear() to prevent ThreadLocal and Log4j key leaks when exceptions occur before endAll() or writePhaseHeader() are reached. This ensures cleanup on all exit paths. core/src/main/java/org/opensearch/sql/executor/QueryService.java [204-243] try { QueryPhaseTracker tracker = QueryPhaseTracker.startOrRestore(); tracker.beginPhase("analyze"); ProfileContext profileContext = QueryProfiling.activate(QueryContext.isProfileEnabled()); ... } catch (Throwable t) { + ... +} finally { + QueryPhaseTracker.clear(); +} Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a resource leak where QueryPhaseTracker remains in ThreadLocal if an exception occurs before endAll() or writePhaseHeader(). Adding a finally block with clear() would ensure cleanup on all exit paths. However, the placement shown may conflict with existing exception handling logic, and the actual implementation would need careful integration with the existing catch block at line 243.	Medium

---

Previous suggestions

Suggestions up to commit dcd396f

Category	Suggestion	Impact
Possible issue	Handle potential UnsupportedOperationException The getThreadAllocatedBytes() method can throw UnsupportedOperationException if thread memory allocation measurement is not enabled or supported. This uncaught exception could crash the query execution. Wrap the call in a try-catch block to handle this gracefully and default to 0 on failure. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [132-135] -activeMemStart = - SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) - : 0; +activeMemStart = 0; +if (SUN_THREAD_MX != null) { + try { + activeMemStart = SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()); + } catch (UnsupportedOperationException e) { + // Memory allocation tracking not supported + } +} Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that getThreadAllocatedBytes() can throw UnsupportedOperationException at runtime. Adding error handling here would prevent query execution failures and aligns with the best-effort nature of memory tracking mentioned in the class documentation.	Medium
	Protect against memory tracking failure The getThreadAllocatedBytes() call in endCurrentPhase() can throw UnsupportedOperationException if memory tracking becomes unavailable during execution. This would terminate the query unexpectedly. Wrap this call in a try-catch block to ensure phase tracking continues even if memory measurement fails. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [146-151] if (SUN_THREAD_MX != null) { - long memElapsed = - SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) - - activeMemStart; - memPhases.merge(activePhase, memElapsed, Long::sum); + try { + long memElapsed = + SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) + - activeMemStart; + memPhases.merge(activePhase, memElapsed, Long::sum); + } catch (UnsupportedOperationException e) { + // Memory tracking became unavailable + } } Suggestion importance[1-10]: 7 __ Why: Similar to suggestion 2, this correctly identifies that getThreadAllocatedBytes() can throw UnsupportedOperationException during endCurrentPhase(). Adding error handling ensures phase tracking continues even if memory measurement fails, which is consistent with the best-effort approach.	Medium
General	Avoid catching Error types The getSunThreadMXBean() method catches NoClassDefFoundError which is an Error, not an Exception. Catching Error types can mask serious JVM issues and is generally discouraged. Consider removing NoClassDefFoundError from the catch clause and only catch UnsupportedOperationException, or handle the class availability check differently using reflection or class loading checks. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [44-53] private static com.sun.management.ThreadMXBean getSunThreadMXBean() { try { if (THREAD_MX instanceof com.sun.management.ThreadMXBean sun) { return sun; } - } catch (NoClassDefFoundError | UnsupportedOperationException e) { + } catch (UnsupportedOperationException e) { // com.sun.management not available on this JVM + } catch (Exception e) { + // Unexpected exception during cast check } return null; } Suggestion importance[1-10]: 4 __ Why: While catching NoClassDefFoundError is generally discouraged, in this specific case it's used for graceful degradation when com.sun.management classes are unavailable on non-HotSpot JVMs. The suggestion to catch Exception instead is incorrect since NoClassDefFoundError is not an Exception. However, the concern about catching Error types is valid for general best practices.	Low
	Add bounds check before substring The substring() operation can throw StringIndexOutOfBoundsException if MAX_ORIGINAL_QUERY_LENGTH is negative or exceeds the string length. Although unlikely with the current constant value, add a bounds check to ensure MAX_ORIGINAL_QUERY_LENGTH is positive and does not exceed queryText.length() before truncating. legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java [167-174] String queryText = sqlRequest.getSql(); if (queryText != null && threadContext.getHeader(QuerySourceHeaders.ORIGINAL_QUERY_HEADER) == null) { - if (queryText.length() > QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH) { + if (queryText.length() > QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH + && QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH > 0) { queryText = queryText.substring(0, QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH); } threadContext.putHeader(QuerySourceHeaders.ORIGINAL_QUERY_HEADER, queryText); } Suggestion importance[1-10]: 3 __ Why: The suggestion adds a redundant check since the condition queryText.length() > QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH already ensures the substring operation is safe. The additional check for MAX_ORIGINAL_QUERY_LENGTH > 0 provides minimal value given the constant is defined as 4096.	Low

Suggestions up to commit c50c128

Category	Suggestion	Impact
Possible issue	Guard putHeader to prevent exceptions Guard each putHeader call with a null-check on getHeader to prevent IllegalArgumentException if the header already exists. OpenSearch's ThreadContext.putHeader throws when the key is present, which can occur in retry or plugin-chaining scenarios. legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java [158-166] -client - .threadPool() - .getThreadContext() - .putHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER, "sql"); -client - .threadPool() - .getThreadContext() - .putHeader( - QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER, java.util.UUID.randomUUID().toString()); +if (client.threadPool().getThreadContext().getHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER) == null) { + client.threadPool().getThreadContext().putHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER, "sql"); +} +if (client.threadPool().getThreadContext().getHeader(QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER) == null) { + client.threadPool().getThreadContext().putHeader( + QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER, java.util.UUID.randomUUID().toString()); +} Suggestion importance[1-10]: 8 __ Why: Valid concern about putHeader throwing IllegalArgumentException if the header already exists. The PPL path correctly guards with null-checks (lines 188-194), while the SQL path does not. This inconsistency could cause failures in retry or plugin-chaining scenarios.	Medium
General	Replace deprecated Thread.getId() call Replace deprecated Thread.currentThread().getId() with Thread.currentThread().threadId(). The getId() method is deprecated since Java 19, and the project targets Java 21. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [132-135] activeMemStart = SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) + ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) : 0; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that Thread.currentThread().getId() is deprecated since Java 19 and should be replaced with threadId() for Java 21 compatibility. However, this is a deprecation warning rather than a critical bug, so the impact is moderate.	Medium
	Prevent ThreadLocal leak on exceptions Add a finally block that calls QueryPhaseTracker.clear() to prevent ThreadLocal and Log4j key leaks when exceptions occur before endAll() or writePhaseHeader() is reached. This ensures cleanup on all exit paths. core/src/main/java/org/opensearch/sql/executor/QueryService.java [204-243] try { QueryPhaseTracker tracker = QueryPhaseTracker.startOrRestore(); tracker.beginPhase("analyze"); ... } catch (Throwable t) { + ... +} finally { + QueryPhaseTracker.clear(); +} Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a potential resource leak where QueryPhaseTracker may not be cleaned up if exceptions occur before endAll() is called. However, the improved_code shows a finally block that would unconditionally clear the tracker, which might interfere with the intended cross-thread propagation via persist(). A more nuanced approach is needed.	Medium

Suggestions up to commit 4985f3d

Category	Suggestion	Impact
General	Validate memory allocation delta Memory allocation can decrease during execution (e.g., due to GC), resulting in negative memElapsed values. Add validation to ensure only non-negative memory deltas are recorded to prevent misleading metrics. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [138-154] public void endCurrentPhase() { if (activePhase != null) { long elapsed = System.nanoTime() - activeStart; phases.merge(activePhase, elapsed, Long::sum); if (CPU_SUPPORTED) { long cpuElapsed = THREAD_MX.getCurrentThreadCpuTime() - activeCpuStart; cpuPhases.merge(activePhase, cpuElapsed, Long::sum); } if (SUN_THREAD_MX != null) { long memElapsed = SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) - activeMemStart; - memPhases.merge(activePhase, memElapsed, Long::sum); + if (memElapsed > 0) { + memPhases.merge(activePhase, memElapsed, Long::sum); + } } activePhase = null; } } Suggestion importance[1-10]: 7 __ Why: Valid concern about negative memElapsed values due to GC. Adding validation prevents misleading metrics, though the impact is moderate since memory tracking is best-effort and may be unavailable on some JVMs.	Medium
	Preserve tracker on header write failure The QueryPhaseTracker.clear() is called even when an exception occurs during header writing, which may clear tracking data before it's successfully persisted. Move the clear operation inside the try block after successful header write to ensure data isn't lost on failure. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [167-192] private void writePhaseHeader() { QueryPhaseTracker tracker = QueryPhaseTracker.current(); if (tracker != null) { try { client .getNodeClient() .ifPresent( nc -> { ThreadContext tc = nc.threadPool().getThreadContext(); String header = tc.getHeader( QuerySourceHeaders.QUERY_PHASES_HEADER); if (header == null) { tc.putHeader( QuerySourceHeaders.QUERY_PHASES_HEADER, tracker.serialize()); } }); + QueryPhaseTracker.clear(); } catch (Exception e) { // Best-effort — don't fail the query if phase header can't be written } - QueryPhaseTracker.clear(); } } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid point about clearing tracker data even on failure. However, the comment indicates this is best-effort behavior, and the current implementation may be intentional to avoid stale data accumulation across requests.	Low
	Remove unnecessary exception type The method catches UnsupportedOperationException but this exception is not thrown during the instanceof check or cast. Remove this unnecessary exception type from the catch clause to avoid confusion and maintain accurate exception handling. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [44-53] private static com.sun.management.ThreadMXBean getSunThreadMXBean() { try { if (THREAD_MX instanceof com.sun.management.ThreadMXBean sun) { return sun; } - } catch (NoClassDefFoundError | UnsupportedOperationException e) { + } catch (NoClassDefFoundError e) { // com.sun.management not available on this JVM } return null; } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that UnsupportedOperationException is not thrown by the instanceof check or cast operation. However, this is a minor code cleanup issue that doesn't affect functionality.	Low

Suggestions up to commit b100945

Category	Suggestion	Impact
General	Fix timing metric calculation order The analyzeMetric.set() call uses analyzeStart which was captured before the analyze phase, but the tracker's endCurrentPhase() is called for the "plan" phase. This creates a timing mismatch where analyzeMetric includes both analyze and plan phases. Move the metric recording to after the analyze phase completes, before starting the plan phase. core/src/main/java/org/opensearch/sql/executor/QueryService.java [236-238] +tracker.endCurrentPhase(); analyzeMetric.set(System.nanoTime() - analyzeStart); -tracker.endCurrentPhase(); -tracker.endAll(); +tracker.beginPhase("plan"); Suggestion importance[1-10]: 9 __ Why: The analyzeMetric is set after both analyze and plan phases complete, causing it to incorrectly include plan phase duration. This creates inaccurate profiling data. The metric should be recorded immediately after the analyze phase ends, before the plan phase begins.	High
	Handle unsupported memory allocation tracking The getThreadAllocatedBytes() method can throw UnsupportedOperationException if thread memory allocation measurement is not supported. This should be wrapped in a try-catch block to prevent query failures when the feature is unavailable, similar to how CPU time is handled with the CPU_SUPPORTED flag. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [132-135] -activeMemStart = - SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) - : 0; +activeMemStart = 0; +if (SUN_THREAD_MX != null) { + try { + activeMemStart = SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()); + } catch (UnsupportedOperationException e) { + // Memory allocation tracking not supported + } +} Suggestion importance[1-10]: 8 __ Why: The getThreadAllocatedBytes() method can throw UnsupportedOperationException at runtime even when SUN_THREAD_MX is not null. Adding try-catch protection prevents query failures when memory allocation tracking is unavailable, consistent with the best-effort approach documented in the class.	Medium
	Protect against memory tracking exceptions The getThreadAllocatedBytes() call in endCurrentPhase() can also throw UnsupportedOperationException. Wrap this call in a try-catch block to ensure phase tracking completes gracefully even when memory tracking becomes unavailable mid-execution. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [146-151] if (SUN_THREAD_MX != null) { - long memElapsed = - SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) - - activeMemStart; - memPhases.merge(activePhase, memElapsed, Long::sum); + try { + long memElapsed = + SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) + - activeMemStart; + memPhases.merge(activePhase, memElapsed, Long::sum); + } catch (UnsupportedOperationException e) { + // Memory allocation tracking not supported + } } Suggestion importance[1-10]: 8 __ Why: Similar to beginPhase(), the getThreadAllocatedBytes() call in endCurrentPhase() can throw UnsupportedOperationException. Wrapping it in try-catch ensures phase tracking completes gracefully and prevents query failures when memory tracking becomes unavailable.	Medium
	Avoid catching Error types The method catches NoClassDefFoundError which is an Error, not an Exception. Catching Error types can mask serious JVM issues and is generally discouraged. Consider removing NoClassDefFoundError from the catch clause and only catch UnsupportedOperationException, or handle the class availability check differently using reflection or class loading checks. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [44-53] private static com.sun.management.ThreadMXBean getSunThreadMXBean() { try { if (THREAD_MX instanceof com.sun.management.ThreadMXBean sun) { return sun; } - } catch (NoClassDefFoundError | UnsupportedOperationException e) { + } catch (UnsupportedOperationException e) { // com.sun.management not available on this JVM } return null; } Suggestion importance[1-10]: 7 __ Why: Catching NoClassDefFoundError is generally discouraged as it can mask serious JVM issues. However, in this specific case it's used defensively to handle the absence of com.sun.management classes on non-HotSpot JVMs, which is a valid use case. The suggestion to use reflection would be more complex without significant benefit.	Medium

Suggestions up to commit 134b552

Category	Suggestion	Impact
Possible issue	Use Java 11-compatible thread ID method The threadId() method was introduced in Java 19. For compatibility with earlier Java versions (11-18), use Thread.currentThread().getId() instead to avoid NoSuchMethodError at runtime. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [132-135] activeMemStart = SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) + ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) : 0; Suggestion importance[1-10]: 10 __ Why: The threadId() method is only available in Java 19+, which will cause NoSuchMethodError on Java 11-18. This is a critical compatibility issue that will break the feature on supported JVM versions.	High
	Fix thread ID retrieval compatibility Replace threadId() with getId() for Java 11-18 compatibility. The threadId() method is only available in Java 19+, which will cause runtime failures on older JVMs. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [146-151] if (SUN_THREAD_MX != null) { long memElapsed = - SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) + SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) - activeMemStart; memPhases.merge(activePhase, memElapsed, Long::sum); } Suggestion importance[1-10]: 10 __ Why: Same critical compatibility issue as suggestion 1. Using threadId() instead of getId() will cause runtime failures on Java versions 11-18, which are likely supported by OpenSearch.	High
General	Ensure tracker exists before ending phases The tracker may be null if executePlan is called without prior phase tracking initialization. Consider using startOrRestore() instead of current() to ensure a tracker exists, or handle the null case more defensively to avoid losing phase data. core/src/main/java/org/opensearch/sql/executor/QueryService.java [352-357] org.opensearch.sql.common.utils.QueryPhaseTracker tracker = - org.opensearch.sql.common.utils.QueryPhaseTracker.current(); -if (tracker != null) { - tracker.endCurrentPhase(); - tracker.endAll(); -} + org.opensearch.sql.common.utils.QueryPhaseTracker.startOrRestore(); +tracker.endCurrentPhase(); +tracker.endAll(); Suggestion importance[1-10]: 3 __ Why: While the suggestion is technically valid, the null check is already present and handles the case appropriately. Using startOrRestore() would create unnecessary tracker instances when no tracking was initiated, potentially adding overhead.	Low

[github-actions]

Persistent review updated to latest commit 8c958a8

[github-actions]

Persistent review updated to latest commit 1e86aa4

[github-actions]

Persistent review updated to latest commit 70cb9db

[github-actions]

Persistent review updated to latest commit a38aeb9

[github-actions]

Persistent review updated to latest commit c3e375a

[github-actions]

Persistent review updated to latest commit 134b552

[ansjcy]

I need to take a closer look into the core logistics but please make sure you format this PR before submitting.

[ansjcy]

please do proper import, don't use org.opensearch.sql.common.utils.QueryPhaseTracker..

[ansjcy]

nit: why is this needed?

[github-actions]

Persistent review updated to latest commit b100945

[github-actions]

Persistent review updated to latest commit 4985f3d

[github-actions]

Persistent review updated to latest commit c50c128

[github-actions]

Persistent review updated to latest commit dcd396f

[github-actions]

Persistent review updated to latest commit c50c128

[dzane17]

Instead of injecting four new request headers, can you add the parent id to the DSL search tasks created by sql/ppl? Then every normal DSL search task will have a reference back to its originating sql/ppl query. Query Insights can simply look up whatever info it needs, like the original request body.

The gap right now is that not all DSL search tasks carry the parent PPL/SQL id. PPL v2 was instrumented recently in #5628 (applyParentTask in OpenSearchNodeClient). If the same were done for the v3 PPL and SQL paths (SQL also needs a coordinator task created first), it would avoid the new headers, getTaskHeaders() wiring, thread-context propagation, and raw query text on the wire that the header approach requires.

[Swiddis]

preliminary review, haven't traced through the low-level details of thread marking and such / why it matters whether or not we're on a sun jvm

[Swiddis]

Is it better to add these phase markers here instead of part of executeStage?

Can possibly rename StageErrorHandler if it's not broad enough to cover all stage-covering monitoring. Same thing with possibly merging the whole queryphasetracker into what we're already using to define these phases.

[Swiddis]

did you mean to put this ai review folder in the project root

[Swiddis]

are these headers correctly getting propagated after a complex thread pool handoff?

[Swiddis]

Instead of injecting four new request headers, can you add the parent id to the DSL search tasks created by sql/ppl? Then every normal DSL search task will have a reference back to its originating sql/ppl query. Query Insights can simply look up whatever info it needs, like the original request body.

The gap right now is that not all DSL search tasks carry the parent PPL/SQL id. PPL v2 was instrumented recently in #5628 (applyParentTask in OpenSearchNodeClient). If the same were done for the v3 PPL and SQL paths (SQL also needs a coordinator task created first), it would avoid the new headers, getTaskHeaders() wiring, thread-context propagation, and raw query text on the wire that the header approach requires.

That pr is mostly focused on v3, not sure where v2 is from. But other than that, I like this proposal, we should be roughly aligning to the existing stage tracking and thread propagation we already have as part of complex/background thread handoff. Particularly because historically thread propagation is a pain to modify and guard on edge cases, I'd be very in favor of a single component that owns "create an instrumented thread context that runs $LAMBDA and emits the right metrics to the right places."

[dzane17]

My bad, then the parent task approach is even simpler. @penghuo @dai-chen do you have any ideas or concerns?