Repository navigation
feat(ppl): Report completed PPL queries to Query Insights Top N - #5760
KishoreKicha14 wants to merge 28 commits into
Conversation
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 9394b37.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
aa97a0c to
c73c5a5
Compare
PR Reviewer Guide 🔍(Review updated until commit 1ccbe68)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 1ccbe68 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit f12f52f
Suggestions up to commit 0125171
Suggestions up to commit 1b09d64
Suggestions up to commit c49b29c
Suggestions up to commit 46062e0
|
c73c5a5 to
d05d99d
Compare
|
Persistent review updated to latest commit d05d99d |
d05d99d to
fae8fa0
Compare
|
Persistent review updated to latest commit fae8fa0 |
fae8fa0 to
f4ba88d
Compare
|
Persistent review updated to latest commit f4ba88d |
f4ba88d to
31fd289
Compare
|
Persistent review updated to latest commit 31fd289 |
31fd289 to
5355c10
Compare
|
Persistent review updated to latest commit 297b6bc |
|
Persistent review updated to latest commit 82ccfbf |
… resource stats Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
Query Insights stamps a native search record with the request's start time (SearchRequest#getOrCreateAbsoluteStartMillis), but the PPL record used the report time. A query that crossed a Top N window boundary was therefore counted in a later window than the child searches it spawned, so the parent-marker roll-up could not find them. Use the task's start time, which core already tracks on Task. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
The parent marker already in the thread context is kept rather than replaced, because putHeader rejects a duplicate key and clearing the context first would drop the authenticated-user headers the child searches need. The record, however, was always built from this task's own id, so it advertised a marker that no child search references while the children's cost rolled up into the outer query's record instead. Skip the record in that case. No in-process caller reaches it today -- the only callers are RestPPLQueryAction and RestPPLGrammarAction, both on a fresh REST thread, and the header is registered via getTaskHeaders rather than getRestHeaders so a client cannot inject it -- but the two sides now agree instead of silently disagreeing. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
82ccfbf to
46062e0
Compare
|
Persistent review updated to latest commit 46062e0 |
The analyze route builds its AST on a path of its own rather than sharing execute's, and nothing exercised it: the query-insights sink it feeds, the no-sink overload, and the parse-failure path were all untested. Add those three, asserting the sink stays untouched when the query never parses. Also cover the exists subquery, which reaches a different visit method than the already-tested in subquery. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
|
Persistent review updated to latest commit c49b29c |
Resolves the overlap with async PPL query submission (opensearch-project#5832), which added a third route to doExecute and a PPLQueryRunner that took the old Consumer<String> anonymized-query sink this branch replaced with Consumer<QueryInsightsMetadata>. Retype PPLQueryRunner's sink, and order the async branch ahead of the Query Insights branch so an async submission is not recorded: it answers RUNNING and the query outlives the task, so a record built from the task's completion would carry near-zero latency and reference child searches that have not run yet. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
|
Persistent review updated to latest commit 1b09d64 |
Both plan() and analyze() parsed the request and collected the source indices from the resulting AST, in two copies of the same block that had to stay in sync. Fold them into a single prepare() that parses once and returns the statement together with the metadata taken from that same AST, so the recorded indices are provably the ones that execute. Each route still supplies its own statement-builder context, which is the only part that differed. The repeated explain-format ternary becomes a helper. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
Neither fallback is reachable through the public API, since the parser only yields a Query or an Explain wrapping one, so cover them directly: make extractIndexNames package-private and assert a non-Query statement and a plan that throws mid-walk both yield no indices. The second is the one that matters: index extraction is best-effort metadata, and a plan that blows up during the walk must not fail the query that is about to execute. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
The walk guards against an absent sub-plan, filter condition, eval list and eval expression, because it produces best-effort metadata: a shape it did not expect must cost the executing query nothing. None are produced by the parser, so build them by hand and assert the walk still reaches the child relation. Also collapse the explain-format check to Strings.isNullOrEmpty. The null half was unreachable -- the REST layer always supplies a string, and the three-argument request constructor defaults it to empty -- so one condition says the same thing without a branch no test can take. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
|
Persistent review updated to latest commit 0125171 |
The record rebuilt the marker from the task id regardless of whether the stamp had succeeded, so a failed stamp produced a record advertising a marker no child search carried. Store the marker on the task only once it is in the thread context and send whatever was stored; the reporter already writes a null marker as empty, which Query Insights treats as a parent with no children. This also removes the duplicate computation. Review cleanup in the same pass: drop the metadataTarget alias of an already-final local; in the dispatcher test remove an unused thread- context stub that only passed under LENIENT and revert the log4j static-import churn it forced; drop two tests of pre-existing dispatcher behaviour and make the third assert the accounting scope closes on the exception path, which is the line this change added; remove a stray blank line; correct the settings doc to 3.10; and reword the security IT so it claims only what it exercises -- Query Insights is not installed there, so the send never runs. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
The reader lives in a different plugin and the two share no classes, so the byte layout is the contract, and nothing on this side exercised the writer. Pin it three ways: a golden payload that fails on any one-sided layout change, a field-by-field decode in the consumer's read order that documents each field and rejects trailing bytes, and the null/negative normalisation. Also pin the send itself: it runs with the caller's context stashed so the internal cluster:admin action is not authorised against the end user, the context is restored afterwards, and a transport failure never reaches the query that just finished. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
|
Persistent review updated to latest commit f12f52f |
Only where and eval conditions were bridged into expressions, so a subquery anywhere else was dropped from the recorded indices. Probing each expression-bearing command against a live cluster showed the gap is real: stats, chart, timechart, fillnull and foreach all parse and execute a scalar subquery in their argument, and the documented join ON form (on l.a = r.a AND r.a in [ source = inner ... ]) was missed as well. Bridge all of them, plus eventstats/streamstats for symmetry. Two walk fixes surfaced doing this. Node.getChild() defaults to null and Alias keeps that default, so descending into any aggregation argument NPEd and the function beneath the alias was never reached; the walk now treats null as no children and visits an alias's delegate. Null-safety is concentrated in one visit() helper so each bridge is a single call. Every plan node holding a plan outside its child list was re-checked: all either override getSources() or expose it through getChild(). The class comment previously claimed join ON was unsupported. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
|
Persistent review updated to latest commit 1ccbe68 |
| throw new AssertionError( | ||
| "join query failed: " + TestUtils.getResponseBody(e.getResponse()), e); | ||
| } | ||
| assertTrue(result.has("datarows")); |
There was a problem hiding this comment.
This IT only proves PPL execution doesn't break when Query Insights is absent — it never asserts the feature itself. Can we land an end-to-end test in this PR that pins the record (indices, parent marker, child DSL rollup) and the serialized payload? Options: co-install Query Insights in a dedicated test module here, link a parallel PR in the Query Insights repo, or add a component test with a fake sink. Shipping a feature where no test in the owning PR asserts its happy-path output is a merge blocker for me. assertTrue(result.has("datarows")) on the join path is "didn't throw," not coverage.
| * does read-side visibility of the recorded identity ({@code | ||
| * search.insights.top_queries.filter_by_mode}). | ||
| */ | ||
| public class QueryInsightsReportingSecurityIT extends SecurityTestBase { |
There was a problem hiding this comment.
Ditto. This IT only proves PPL execution doesn't break when Query Insights is absent.
The existing ITs only prove PPL still runs when Query Insights is absent; nothing asserted that a record is written. Add a cluster with Query Insights installed and an IT that checks, against the real plugin: - a query is recorded with query_source=PPL, its indices, a <source>:<nodeId>:<taskId> parent marker and non-zero cpu/memory - a missing index and an unknown field are recorded as failed - explain (endpoint and inline), syntax errors and async submissions write nothing; neither does a query with the gate off, while its search is still recorded as an ordinary top-level query - every search a join spawns is filed as a DSL child of the record, reads the indices the record names, and is not stamped earlier than it Each scenario reads its own index, since recorded text is anonymized, and absence is asserted only after a later record has arrived. Runs via :integ-test:integTestWithQueryInsights and stays out of check until the published Query Insights snapshot includes the PPL consumer; until then pass -PqueryInsightsZip=<local build>. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
Add a cluster with Query Insights and the security plugin, and an IT that checks the identity a record carries and who can read it back: - a user with PPL access but no Query Insights permission at all, including the cluster:admin report action, still gets a record, and it carries their username and roles - that user gets 403 on the Top N API - with filter_by_mode=username, two users each see their own queries and not the other's Removing the identity capture fails the first and third tests. Move the record-reading helpers the two Query Insights ITs share into QueryInsightsRecords, and the PPL index permissions into a constant on SecurityTestBase so new roles grant what the existing ones do. Runs via :integ-test:integTestWithQueryInsightsAndSecurity, outside check for the same reason as integTestWithQueryInsights. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com> # Conflicts: # plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java # ppl/src/main/java/org/opensearch/sql/ppl/PPLQueryRunner.java
The record carried the indices a PPL query reads, resolved by walking its AST through a getSources() hook added to Join, Lookup, Append, AppendCol, AppendPipe and GraphLookup. Nothing needs them: the Top N use cases are slow and failed queries. The searches a query runs are already recorded as its children, each with the indices it reads, so Query Insights can show those under the query instead. Remove the extractor, the getSources() hook, and the indices from the record and its wire format. Neither plugin has released the format, so it stays at version 1; the Query Insights handler has to stop reading the field at the same time. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
The Top N record carried the anonymized query text, with every literal masked, which hides what makes a slow or failed query diagnosable: which source and which filter values. Query Insights stores a DSL search's source as sent, literals included, and its RBAC filter (search.insights.top_queries.filter_by_mode) governs who can read either. Record the query as submitted. The tracing span and the request log line keep the anonymized text. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
The PPL record no longer carries indices and now keeps the query as written, so the ITs find a record by the index its query text reads, assert the recorded text exactly, and check that a join's searches, rather than the record, carry the join's two indices. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
The engine measured its own threads privately for the Top N record, so the PPL task's resource_stats stayed at zero, and _tasks and Live Queries showed no CPU or memory for work done inside the engine. Drive the engine's thread scopes through core's TaskResourceTrackingService instead, and mark PPLQueryTask as supporting resource tracking. Core then registers the task when task_resource_tracking.enabled is on, records the engine threads in its resource_stats, and refreshes threads still running whenever tasks are listed. The Top N record reads the same totals. Scopes now run on every execute route, not only when Query Insights recording is on, since _tasks and Live Queries read the task either way. The background scan thread is scoped too, so its work is no longer missed. The record's latency is now taken when the response is sent. The record goes out once the last scope closes, and a lookahead search still running on the background pool after the query has answered would otherwise count toward it. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
QueryInsightsParentHeaderScopeTest re-implemented the thread-context steps doExecute takes and tested that copy, so it would keep passing if doExecute changed. Replace it with a test that calls doExecute with only the engine stubbed and core's task resource tracking real. The test decodes the record sent to Query Insights and checks each field: the query text as submitted, the parent marker, node, start timestamp, user, CPU and memory equal to the task's totals, and the failed flag. It also checks that the marker reaches the engine's thread but not the calling thread or the caller's callback, that a nested query keeps the outer marker and sends nothing, that nothing is sent with recording off or Query Insights absent while CPU is still tracked, and that latency stops at the response when a lookahead search keeps running after it. Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>
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
--signoffor-s.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.