feat(ppl): Report completed PPL queries to Query Insights Top N

[KishoreKicha14]

Description

PPL queries don't currently show up in Query Insights (only DSL queries do). This wires PPL into the same Top N reporting so you can see PPL query latency/CPU/memory next to everything else.

When a PPL query finishes, we send a record to Query Insights with the source (PPL), the indices it read, and the total latency/CPU/memory (from the task resource tracking framework, same numbers OpenSearch already tracks per task).

A PPL query can fan out into multiple DSL searches under the hood — a join, for example, scans each table separately. We tag those child searches with a header (X-Query-Insights-Parent) pointing back at the PPL query that spawned them, so Query Insights can show them as sub-queries on the detail page. Getting the tag to survive was the fiddly part: the engine hops the query across a couple of thread pools (worker → complex-worker → background scan), and the header doesn't ride along automatically, so we carry it on a ThreadLocal and re-stamp it before each background search.

Screenshots:

Query Overview Page

Query Detail Page with DSL subqueries derived

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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 393b517.

Path	Line	Severity	Description
plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java	290	medium	writeQueryInsightsRecord() captures and transmits the full raw query text (including WHERE clause predicates) to the Query Insights transport handler. PPL/SQL predicates frequently contain filter values that may include PII or sensitive literals. While this is cluster-internal transport and an intended monitoring feature, the full query text is serialized without redaction, making sensitive values visible in any Query Insights storage or log.
plugin/src/main/java/org/opensearch/sql/plugin/transport/QueryInsightsReporter.java	96	low	All exceptions in report() are caught and logged only at DEBUG level, silently suppressing transport failures. An attacker who registers a rogue action under the same name cluster:admin/opensearch/query_insights/report_query_bytes would receive query records without any warning visible to operators, since errors are completely swallowed.
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java	33	low	com.sun.management.ThreadMXBean is an internal, non-public JDK API (sun.* namespace). Its availability is not guaranteed across JVM distributions. The fallback to null is silent (WARN level only), meaning resource tracking silently disables on non-HotSpot JVMs with no operator visibility.
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java	74	low	QUERY_INSIGHTS_PARENT_HEADER is defined as a literal 'X-Query-Insights-Parent' duplicated from QueryInsightsMarker.PARENT_HEADER with a comment requiring manual synchronization. If the two constants diverge (e.g., in a partial upgrade), background scan child tasks will carry a different header than the registered task header, silently breaking the parent-child association in Query Insights without any compile-time or runtime error.

The table above displays the top 10 most important findings.

Total: 4 | Critical: 0 | High: 0 | Medium: 1 | Low: 3

---

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 393b517)

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 THREAD_MX_BEAN is cast from java.lang.management.ThreadMXBean to com.sun.management.ThreadMXBean without verifying the cast is valid. If the JVM provides a ThreadMXBean implementation that is not a com.sun.ThreadMXBean, the cast will succeed at compile time but fail at runtime with a ClassCastException. The instanceof check on line 41 only confirms the bean exists, not that it is the Sun-specific subclass. This will cause resource tracking to silently fail (caught and logged) on non-HotSpot JVMs or custom implementations. private static ThreadMXBean resolveThreadMXBean() { try { java.lang.management.ThreadMXBean bean = ManagementFactory.getThreadMXBean(); if (bean instanceof ThreadMXBean) { return (ThreadMXBean) bean; } } catch (Exception e) { LOG.warn("Per-thread resource metrics unavailable; PPL task resource tracking disabled", e); } return null; } Possible Issue searchWithParentHeader uses ThreadContext.newStoredContext(true) to snapshot and restore the context around adding the parent header. However, the true parameter preserves response headers, which may cause unintended header accumulation or leakage across multiple searches if the same thread is reused. The comment states it avoids dropping the security user transient, but newStoredContext(true) specifically preserves response headers, not transients. If the goal is to preserve transients (like the security user), this approach may not achieve it, and the security context could still be lost. try (ThreadContext.StoredContext ignored = threadContext.newStoredContext(true)) { threadContext.putHeader(QUERY_INSIGHTS_PARENT_HEADER, parentMarker); return client.search(request); } } Possible Issue extractPplIndices uses a regex with alternation and quantifiers that could exhibit quadratic behavior on certain inputs despite the comment claiming no catastrophic backtracking. The pattern (\"[^\"]*\"|'[^']*'|[^]*|[^\\s|]+)with the outerwhile (m.find())loop can cause the matcher to repeatedly scan overlapping portions of the input when quotes are unbalanced or interleaved with commas. The test at line 93 uses 50,000 repetitions ofa,"but does not close the quotes, which may not trigger the worst-case scenario. A more hostile input likesource=followed by 50,000a"` (unbalanced quotes without commas) could cause the regex engine to backtrack excessively, leading to timeouts. final java.util.regex.Matcher m = java.util.regex.Pattern.compile( "(?i)\\b(?:source|index)\\s*=\\s*(\"[^\"]*\"|'[^']*'|`[^`]*`|[^\\s|]+)") .matcher(queryText); while (m.find()) {

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 393b517

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add warning for unsupported bean type The method silently returns null when the bean is not an instance of com.sun.management.ThreadMXBean, which may cause resource tracking to fail unexpectedly on non-HotSpot JVMs. Add a warning log when the bean type check fails to help diagnose why resource tracking is disabled. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java [38-48] private static ThreadMXBean resolveThreadMXBean() { try { java.lang.management.ThreadMXBean bean = ManagementFactory.getThreadMXBean(); if (bean instanceof ThreadMXBean) { return (ThreadMXBean) bean; } + LOG.warn("ThreadMXBean is not com.sun.management.ThreadMXBean; PPL task resource tracking disabled"); } catch (Exception e) { LOG.warn("Per-thread resource metrics unavailable; PPL task resource tracking disabled", e); } return null; } Suggestion importance[1-10]: 5 __ Why: Adding a warning when the bean type check fails helps diagnose why resource tracking is disabled on non-HotSpot JVMs. This improves observability but doesn't fix a critical issue.	Low
	Warn on negative resource metrics Negative values for latencyMillis, cpuNanos, or memoryBytes indicate a measurement error or clock skew. Instead of silently clamping to zero, log a warning when negative values are detected so operators can identify and investigate the root cause. plugin/src/main/java/org/opensearch/sql/plugin/transport/QueryInsightsReporter.java [86-88] +if (latencyMillis < 0 || cpuNanos < 0 || memoryBytes < 0) { + LOG.warn("Negative resource metrics detected: latency={}, cpu={}, memory={}", latencyMillis, cpuNanos, memoryBytes); +} out.writeVLong(Math.max(0L, latencyMillis)); out.writeVLong(Math.max(0L, cpuNanos)); out.writeVLong(Math.max(0L, memoryBytes)); Suggestion importance[1-10]: 4 __ Why: Logging negative metrics helps identify measurement errors or clock skew issues. However, this is a minor observability improvement rather than a critical fix, as the values are already clamped to zero.	Low

---

Previous suggestions

Suggestions up to commit 5355c10

Category	Suggestion	Impact
General	Validate parent marker consistency The method checks if the header is already set but doesn't verify if it matches the expected parentMarker. If a different parent marker is already present, the search will proceed with the wrong parent association, causing incorrect query attribution in Query Insights. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [168-182] private OpenSearchResponse searchWithParentHeader( OpenSearchRequest request, @Nullable String parentMarker) { if (parentMarker == null || parentMarker.isEmpty() || client.getNodeClient().isEmpty()) { return client.search(request); } ThreadContext threadContext = client.getNodeClient().get().threadPool().getThreadContext(); - // Already set on this pool thread: leave it be, putHeader would throw on a duplicate. - if (threadContext.getHeader(QUERY_INSIGHTS_PARENT_HEADER) != null) { + String existingHeader = threadContext.getHeader(QUERY_INSIGHTS_PARENT_HEADER); + if (existingHeader != null) { + if (!existingHeader.equals(parentMarker)) { + LOG.warn("Parent marker mismatch: expected [{}], found [{}]", parentMarker, existingHeader); + } return client.search(request); } try (ThreadContext.StoredContext ignored = threadContext.newStoredContext(true)) { threadContext.putHeader(QUERY_INSIGHTS_PARENT_HEADER, parentMarker); return client.search(request); } } Suggestion importance[1-10]: 7 __ Why: Detecting a parent marker mismatch is important for correctness in Query Insights attribution. The warning would help identify cases where the wrong parent association occurs, though the scenario may be rare in practice.	Medium
	Log negative resource metrics Negative values for latency, CPU, or memory metrics indicate a serious data integrity issue that should be logged before being clamped to zero. Silently clamping masks potential bugs in resource tracking calculations. plugin/src/main/java/org/opensearch/sql/plugin/transport/QueryInsightsReporter.java [86-88] +if (latencyMillis < 0) { + LOG.warn("Negative latency detected: {}", latencyMillis); +} +if (cpuNanos < 0) { + LOG.warn("Negative CPU nanos detected: {}", cpuNanos); +} +if (memoryBytes < 0) { + LOG.warn("Negative memory bytes detected: {}", memoryBytes); +} out.writeVLong(Math.max(0L, latencyMillis)); out.writeVLong(Math.max(0L, cpuNanos)); out.writeVLong(Math.max(0L, memoryBytes)); Suggestion importance[1-10]: 6 __ Why: Logging negative values before clamping helps identify data integrity issues in resource tracking. While useful for debugging, the impact is moderate since the values are already being sanitized with Math.max(0L, ...).	Low
	Add warning for unsupported bean type The method silently returns null when the bean is not an instance of com.sun.management.ThreadMXBean, which could lead to unexpected behavior. Add a warning log when the bean type doesn't match to help diagnose why resource tracking is disabled. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java [38-48] private static ThreadMXBean resolveThreadMXBean() { try { java.lang.management.ThreadMXBean bean = ManagementFactory.getThreadMXBean(); if (bean instanceof ThreadMXBean) { return (ThreadMXBean) bean; } + LOG.warn("ThreadMXBean is not com.sun.management.ThreadMXBean; PPL task resource tracking disabled"); } catch (Exception e) { LOG.warn("Per-thread resource metrics unavailable; PPL task resource tracking disabled", e); } return null; } Suggestion importance[1-10]: 5 __ Why: Adding a warning when the bean type doesn't match ThreadMXBean improves observability by helping diagnose why resource tracking is disabled. However, this is a minor enhancement since the existing catch-all exception handler already logs failures.	Low

Suggestions up to commit 31fd289

Category	Suggestion	Impact
General	Cache compiled regex pattern statically Compiling the regex pattern on every call to extractPplIndices is inefficient. Consider declaring the pattern as a static final field to compile it once and reuse it across all invocations, improving performance for repeated queries. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java [358-361] -final java.util.regex.Matcher m = +private static final java.util.regex.Pattern PPL_INDEX_PATTERN = java.util.regex.Pattern.compile( - "(?i)\\b(?:source|index)\\s*=\\s*(\"[^\"]*\"|'[^']*'|`[^`]*`|[^\\s|]+)") - .matcher(queryText); + "(?i)\\b(?:source|index)\\s*=\\s*(\"[^\"]*\"|'[^']*'|`[^`]*`|[^\\s|]+)"); +static java.util.List<String> extractPplIndices(String queryText) { + ... + final java.util.regex.Matcher m = PPL_INDEX_PATTERN.matcher(queryText); + ... +} + Suggestion importance[1-10]: 6 __ Why: Caching the compiled regex pattern as a static final field is a valid performance optimization that avoids recompiling the pattern on every query. This is a good practice for frequently-used patterns, though the impact is relatively minor since pattern compilation is not extremely expensive.	Low
	Add logging for non-matching bean type The method silently returns null when the bean is not an instance of com.sun.management.ThreadMXBean, which could lead to unexpected behavior. Consider logging a warning in this case to inform operators that resource tracking will be disabled, similar to the exception handler. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java [42-52] private static ThreadMXBean resolveThreadMXBean() { try { java.lang.management.ThreadMXBean bean = ManagementFactory.getThreadMXBean(); if (bean instanceof ThreadMXBean) { return (ThreadMXBean) bean; } + LOG.warn("ThreadMXBean is not com.sun.management.ThreadMXBean; PPL task resource tracking disabled"); } catch (Exception e) { LOG.warn("Per-thread resource metrics unavailable; PPL task resource tracking disabled", e); } return null; } Suggestion importance[1-10]: 5 __ Why: Adding a warning log when the bean is not an instance of com.sun.management.ThreadMXBean improves observability by making it explicit that resource tracking is disabled. However, this is a minor enhancement since the method already logs exceptions and returns null gracefully, which is handled throughout the code.	Low

Suggestions up to commit f4ba88d

Category	Suggestion	Impact
General	Cache compiled regex pattern The regex pattern is compiled on every call to extractPplIndices, which is inefficient. Consider compiling the pattern once as a static final field to avoid repeated compilation overhead, especially since this method may be called frequently for query processing. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java [357-361] -final java.util.regex.Matcher m = +private static final java.util.regex.Pattern PPL_INDEX_PATTERN = java.util.regex.Pattern.compile( - "(?i)\\b(?:source|index)\\s*=\\s*" - + "((?:\"[^\"]*\"|'[^']*'|`[^`]*`|[^\\s,|]+)(?:\\s*,\\s*(?:\"[^\"]*\"|'[^']*'|`[^`]*`|[^\\s,|]+))*)") - .matcher(queryText); + "(?i)\\b(?:source|index)\\s*=\\s*" + + "((?:\"[^\"]*\"|'[^']*'|`[^`]*`|[^\\s,|]+)(?:\\s*,\\s*(?:\"[^\"]*\"|'[^']*'|`[^`]*`|[^\\s,|]+))*)"); +static java.util.List<String> extractPplIndices(String queryText) { + ... + final java.util.regex.Matcher m = PPL_INDEX_PATTERN.matcher(queryText); + ... +} + Suggestion importance[1-10]: 7 __ Why: Compiling the regex pattern once as a static final field is a good performance optimization, especially since extractPplIndices may be called frequently. This reduces unnecessary overhead from repeated pattern compilation and is a standard best practice for regex usage.	Medium
	Log when bean type mismatches The method silently returns null when the bean is not an instance of com.sun.management.ThreadMXBean, which could lead to unexpected behavior. Consider logging a warning when the bean type doesn't match, similar to the exception case, so operators are aware that resource tracking is degraded. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java [42-52] private static ThreadMXBean resolveThreadMXBean() { try { java.lang.management.ThreadMXBean bean = ManagementFactory.getThreadMXBean(); if (bean instanceof ThreadMXBean) { return (ThreadMXBean) bean; } + LOG.warn("ThreadMXBean is not com.sun.management.ThreadMXBean; PPL task resource tracking disabled"); } catch (Exception e) { LOG.warn("Per-thread resource metrics unavailable; PPL task resource tracking disabled", e); } return null; } Suggestion importance[1-10]: 5 __ Why: Adding a warning log when the ThreadMXBean type doesn't match improves observability by making operators aware that resource tracking is degraded. However, this is a minor enhancement that doesn't fix a bug or address a critical issue.	Low
	Defer thread ID capture conditionally The trackedThreadId is captured before checking if trackResources is true, which means the thread ID is retrieved even when tracking is disabled. Consider moving the thread ID capture inside the conditional block to avoid unnecessary work when resource tracking is not supported. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java [162-166] final boolean trackResources = cancelTask != null && cancelTask.supportsResourceTracking(); -final long trackedThreadId = Thread.currentThread().getId(); -final boolean trackingStarted = - trackResources && startThreadResourceTracking(cancelTask, trackedThreadId); +final boolean trackingStarted; +final long trackedThreadId; +if (trackResources) { + trackedThreadId = Thread.currentThread().getId(); + trackingStarted = startThreadResourceTracking(cancelTask, trackedThreadId); +} else { + trackedThreadId = -1L; + trackingStarted = false; +} Suggestion importance[1-10]: 3 __ Why: While deferring Thread.currentThread().getId() until after the trackResources check is a micro-optimization, the performance gain is negligible since getId() is a very lightweight operation. The suggestion is technically correct but offers minimal practical benefit.	Low

Suggestions up to commit fae8fa0

Category	Suggestion	Impact
General	Cache compiled regex pattern The regex pattern is compiled on every method invocation, which is inefficient for a method that may be called frequently. Consider compiling the pattern once as a static final field to improve performance and reduce overhead. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java [357-361] -java.util.List<String> extractPplIndices(String queryText) { +private static final java.util.regex.Pattern PPL_INDEX_PATTERN = + java.util.regex.Pattern.compile( + "(?i)\\b(?:source|index)\\s*=\\s*" + + "((?:\"[^\"]*\"|'[^']*'|`[^`]*`|[^\\s,|]+)(?:\\s*,\\s*(?:\"[^\"]*\"|'[^']*'|`[^`]*`|[^\\s,|]+))*)"); + +static java.util.List<String> extractPplIndices(String queryText) { final java.util.LinkedHashSet<String> indices = new java.util.LinkedHashSet<>(); if (queryText == null || queryText.isEmpty()) { return new java.util.ArrayList<>(indices); } - final java.util.regex.Matcher m = - java.util.regex.Pattern.compile( - "(?i)\\b(?:source|index)\\s*=\\s*" - + "((?:\"[^\"]*\"|'[^']*'|`[^`]*`|[^\\s,|]+)(?:\\s*,\\s*(?:\"[^\"]*\"|'[^']*'|`[^`]*`|[^\\s,|]+))*)") - .matcher(queryText); + final java.util.regex.Matcher m = PPL_INDEX_PATTERN.matcher(queryText); ... } Suggestion importance[1-10]: 7 __ Why: Compiling the regex pattern once as a static field instead of on every invocation is a meaningful performance optimization, especially if extractPplIndices is called frequently. This reduces overhead and improves efficiency without changing functionality.	Medium
	Log when bean type is incompatible The method silently returns null when the bean is not an instance of com.sun.management.ThreadMXBean, which could lead to unexpected behavior. Consider logging a warning in this case as well, similar to the exception handler, to make it clear why resource tracking is disabled. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java [42-52] private static ThreadMXBean resolveThreadMXBean() { try { java.lang.management.ThreadMXBean bean = ManagementFactory.getThreadMXBean(); if (bean instanceof ThreadMXBean) { return (ThreadMXBean) bean; } + LOG.warn("ThreadMXBean is not com.sun.management.ThreadMXBean; PPL task resource tracking disabled"); } catch (Exception e) { LOG.warn("Per-thread resource metrics unavailable; PPL task resource tracking disabled", e); } return null; } Suggestion importance[1-10]: 5 __ Why: Adding a warning log when the bean type is incompatible improves observability by making it explicit why resource tracking is disabled. However, this is a minor enhancement that doesn't fix a bug or address a critical issue.	Low
	Avoid unnecessary thread ID capture The trackedThreadId is captured even when trackResources is false, which is unnecessary. Consider moving the thread ID capture inside the condition where it's actually used to avoid the overhead of calling Thread.currentThread().getId() when tracking is disabled. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java [162-166] final boolean trackResources = cancelTask != null && cancelTask.supportsResourceTracking(); -final long trackedThreadId = Thread.currentThread().getId(); -final boolean trackingStarted = - trackResources && startThreadResourceTracking(cancelTask, trackedThreadId); +final boolean trackingStarted; +final long trackedThreadId; +if (trackResources) { + trackedThreadId = Thread.currentThread().getId(); + trackingStarted = startThreadResourceTracking(cancelTask, trackedThreadId); +} else { + trackedThreadId = -1L; + trackingStarted = false; +} Suggestion importance[1-10]: 4 __ Why: The suggestion optimizes by avoiding Thread.currentThread().getId() when tracking is disabled. While this is a valid micro-optimization, the performance impact is negligible since getId() is a very lightweight operation, making this a low-priority improvement.	Low

Suggestions up to commit d05d99d

Category	Suggestion	Impact
General	Log when ThreadMXBean type mismatch occurs The method silently returns null when the bean is not an instance of com.sun.management.ThreadMXBean, which could lead to unexpected behavior. Consider logging a warning in this case to inform operators that resource tracking will be disabled, similar to the exception handler. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java [42-52] private static ThreadMXBean resolveThreadMXBean() { try { java.lang.management.ThreadMXBean bean = ManagementFactory.getThreadMXBean(); if (bean instanceof ThreadMXBean) { return (ThreadMXBean) bean; } + LOG.warn("ThreadMXBean is not com.sun.management.ThreadMXBean; PPL task resource tracking disabled"); } catch (Exception e) { LOG.warn("Per-thread resource metrics unavailable; PPL task resource tracking disabled", e); } return null; } Suggestion importance[1-10]: 5 __ Why: Adding a warning log when bean is not an instance of ThreadMXBean improves observability by making it explicit why resource tracking is disabled. However, this is a minor enhancement since the method already logs exceptions and returns null gracefully, and the impact on functionality is minimal.	Low
	Handle potential header-already-exists exception The method calls threadContext.putHeader() which can throw IllegalArgumentException if the header already exists. While the stored context is stashed to prevent this, a race condition could still occur if another thread sets the header. Wrap the putHeader call in a try-catch to handle this edge case gracefully. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [184-194] private OpenSearchResponse searchWithParentHeader( OpenSearchRequest request, @Nullable String parentMarker) { if (parentMarker == null || parentMarker.isEmpty() || client.getNodeClient().isEmpty()) { return client.search(request); } ThreadContext threadContext = client.getNodeClient().get().threadPool().getThreadContext(); try (ThreadContext.StoredContext ignored = threadContext.stashContext()) { - threadContext.putHeader(QUERY_INSIGHTS_PARENT_HEADER, parentMarker); + try { + threadContext.putHeader(QUERY_INSIGHTS_PARENT_HEADER, parentMarker); + } catch (IllegalArgumentException e) { + // Header already exists; proceed without setting it + } return client.search(request); } } Suggestion importance[1-10]: 3 __ Why: While the suggestion addresses a theoretical race condition, the code already uses stashContext() to isolate the thread context, making the race condition extremely unlikely. The added try-catch for IllegalArgumentException provides marginal defensive value but does not address a critical issue.	Low
	Handle race condition in header setting The method checks if the header is null before calling putHeader, but putHeader can still throw IllegalArgumentException if the header exists (race condition). The outer try-catch will swallow this, but it's better to explicitly handle the IllegalArgumentException separately to distinguish it from other failures. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java [173-192] private void stampQueryInsightsParentHeader(PPLQueryTask pplQueryTask) { try { org.opensearch.common.util.concurrent.ThreadContext threadContext = clientRef.threadPool().getThreadContext(); String value = QueryInsightsMarker.value( "PPL", clusterServiceRef.localNode().getId(), pplQueryTask.getId()); if (threadContext.getHeader(QueryInsightsMarker.PARENT_HEADER) == null) { - threadContext.putHeader(QueryInsightsMarker.PARENT_HEADER, value); + try { + threadContext.putHeader(QueryInsightsMarker.PARENT_HEADER, value); + } catch (IllegalArgumentException e) { + // Header was set by another thread between check and put; ignore + } } org.opensearch.sql.opensearch.executor.OpenSearchQueryManager.setQueryInsightsParentMarker( value); } catch (Exception e) { LOG.warn("Failed to stamp Query Insights parent header for query association", e); } } Suggestion importance[1-10]: 3 __ Why: The suggestion adds explicit handling for IllegalArgumentException to distinguish it from other exceptions. However, the outer try-catch already swallows all exceptions gracefully, and the check-then-set pattern with the null check makes the race condition unlikely. This is a minor defensive improvement with limited practical impact.	Low

[github-actions]

Persistent review updated to latest commit d05d99d

[github-actions]

Persistent review updated to latest commit fae8fa0

[github-actions]

Persistent review updated to latest commit f4ba88d

[github-actions]

Persistent review updated to latest commit 31fd289

[github-actions]

Persistent review updated to latest commit 5355c10

[github-actions]

Persistent review updated to latest commit 393b517