Advanced Query Processing Architecture#7
Closed
ShashankFC wants to merge 1 commit into
Closed
Conversation
…7534) * Loki query splitting: interpolate queries before execution * Update tests * Prettier * shardQuerySplitting: remove unnecessary call
Author
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:M
Sequence DiagramThe PR ensures template variables are applied per-query before any splitting or shard lookups. This sequence shows a query request -> per-query interpolation -> shard-aware splitting (using the interpolated selector) -> execution of split queries and returning combined results. sequenceDiagram
participant Client
participant LokiDatasource
participant QuerySplitter
participant LanguageProvider
participant LokiBackend
Client->>LokiDatasource: Send DataQueryRequest (targets with template vars)
LokiDatasource->>LokiDatasource: applyTemplateVariables(each target) -> interpolated queries
LokiDatasource->>QuerySplitter: runSplitQuery / runShardSplitQuery with interpolated queries
QuerySplitter->>LanguageProvider: fetchLabelValues(__stream_shard__, streamSelector=interpolated selector)
QuerySplitter->>LokiDatasource: runQuery(split target with interpolated expr & step)
LokiDatasource->>LokiBackend: Execute query request
LokiBackend-->>LokiDatasource: Return query results
LokiDatasource->>Client: Return combined/merged response
Generated by CodeAnt AI |
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const queries = request.targets | ||
| .filter((query) => !query.hide) | ||
| .filter((query) => query.expr) | ||
| .map((query) => datasource.applyTemplateVariables(query, request.scopedVars, request.filters)); |
There was a problem hiding this comment.
Suggestion: The splitting logic now pre-applies applyTemplateVariables with request.filters, but datasource.runQuery (via DataSourceWithBackend.query) will apply applyTemplateVariables again with the same filters, causing ad hoc filters to be injected twice into the split Loki queries; this can lead to duplicated label filters and unintended query semantics, so the pre-interpolation used only for splitting decisions should not pass request.filters. [logic error]
Severity Level: Major ⚠️
- ❌ Split Loki queries send ad hoc filters twice.
- ⚠️ Shard-split queries pre-apply and re-apply ad hoc filters.
- ⚠️ Split and non-split query expressions become inconsistent.
Suggested change
| .map((query) => datasource.applyTemplateVariables(query, request.scopedVars, request.filters)); | |
| .map((query) => datasource.applyTemplateVariables(query, request.scopedVars)); |
Steps of Reproduction ✅
1. In `public/app/features/query/state/PanelQueryRunner.ts:335`, a dashboard or panel
query against a Loki datasource with ad hoc filters sets `request.filters =
this.templateSrv.getAdhocFilters(ds.name, true);`, so `request.filters` contains one or
more `AdHocVariableFilter` entries.
2. The same request is passed to `LokiDatasource.query()` in
`public/app/plugins/datasource/loki/datasource.ts:299-344`, where, if
`config.featureToggles.lokiQuerySplitting` is enabled and
`requestSupportsSplitting(fixedRequest.targets)` is true, the code executes `return
runSplitQuery(this, fixedRequest);` (line ~338), using `fixedRequest` which still carries
`request.filters`.
3. Inside `runSplitQuery()` in
`public/app/plugins/datasource/loki/querySplitting.ts:291-303`, the code builds `queries`
as:
`request.targets.filter(...).filter(...).map((query) =>
datasource.applyTemplateVariables(query, request.scopedVars, request.filters));`
so `LokiDatasource.applyTemplateVariables()` (defined in `datasource.ts:1110-1141`) is
invoked once here with `adhocFilters = request.filters`, which appends those filters to
`target.expr` via `addAdHocFilters()`.
4. `runSplitQuery()` then creates grouped `requests` whose `targets` are these
already-interpolated queries and calls `runSplitGroupedQueries()`, which in turn calls
`datasource.runQuery(subRequest)` (querySplitting.ts:180). `LokiDatasource.runQuery()` at
`datasource.ts:350-357` delegates to `super.query(fixedRequest)`. In
`DataSourceWithBackend.query()`
(`packages/grafana-runtime/src/utils/DataSourceWithBackend.ts:137-192`), each target is
transformed with:
`...(shouldApplyTemplateVariables ? this.applyTemplateVariables(q, request.scopedVars,
request.filters) : q)`, so `applyTemplateVariables()` is called a second time with the
same `request.filters`. Because `applyTemplateVariables()` again calls
`addAdHocFilters()` on an expression that already includes those filters, each ad hoc
filter is injected twice into the final LogQL selector sent to the backend. This double
injection only happens on the splitting path; non-splitting queries call
`applyTemplateVariables()` exactly once via `DataSourceWithBackend.query()`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** public/app/plugins/datasource/loki/querySplitting.ts
**Line:** 299:299
**Comment:**
*Logic Error: The splitting logic now pre-applies `applyTemplateVariables` with `request.filters`, but `datasource.runQuery` (via `DataSourceWithBackend.query`) will apply `applyTemplateVariables` again with the same filters, causing ad hoc filters to be injected twice into the split Loki queries; this can lead to duplicated label filters and unintended query semantics, so the pre-interpolation used only for splitting decisions should not pass `request.filters`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| .filter((query) => query.expr) | ||
| .filter((query) => !query.hide); | ||
| .filter((query) => !query.hide) | ||
| .map((query) => datasource.applyTemplateVariables(query, request.scopedVars, request.filters)); |
There was a problem hiding this comment.
Suggestion: Similar to time-based splitting, shard splitting now calls applyTemplateVariables with request.filters before delegating to runSplitQuery, but runSplitQuery's sub-requests are executed via datasource.runQuery, which applies applyTemplateVariables again with the same filters, resulting in duplicated ad hoc label filters on sharded queries; using filters only at execution time avoids double application while still letting shard lookup see an interpolated selector. [logic error]
Severity Level: Major ⚠️
- ⚠️ Loki shard-split queries duplicate ad hoc label filters.
- ⚠️ Affects `LokiDatasource.query` when sharding feature enabled.
- ⚠️ Extra filters may slow backend Loki query evaluation.
- ⚠️ Shard label lookups still function but queries are bloated.
Suggested change
| .map((query) => datasource.applyTemplateVariables(query, request.scopedVars, request.filters)); | |
| .map((query) => datasource.applyTemplateVariables(query, request.scopedVars)); |
Steps of Reproduction ✅
1. Enable Loki shard splitting so `LokiDatasource.query()` routes through
`runShardSplitQuery()` when sharding is supported: see
`public/app/plugins/datasource/loki/datasource.ts:299-344`, especially the branch at
`datasource.ts:336-338` calling `runShardSplitQuery(this, fixedRequest)` when
`config.featureToggles.lokiShardSplitting` is true and
`requestSupportsSharding(fixedRequest.targets)` is satisfied.
2. Issue a Loki dashboard/explore query that (a) supports sharding and (b) has ad hoc
filters populated into `request.filters` (standard Grafana ad hoc UI). At runtime this
yields a `DataQueryRequest<LokiQuery>` where `filters` is non-empty, as exercised in
`datasource.test.ts` around the "When using adhoc filters" and `addAdHocFilters` tests
(`datasource.test.ts:229-307, 1232-1315`) and where `targets` contain a log or
metric-over-logs expression.
3. In `runShardSplitQuery()` (`shardQuerySplitting.ts:48-55`), the code builds `queries`
from `request.targets` and calls `datasource.applyTemplateVariables(query,
request.scopedVars, request.filters)` for each target (line 52).
`LokiDatasource.applyTemplateVariables()` (`datasource.ts:1110-1140`) internally calls
`addAdHocFilters()` (`datasource.ts:1073-1091`), which in turn uses `addLabelToQuery()`
(`modifyQuery.ts:147-213`) to splice ad hoc filters into the query expression. For log
queries with a parser (e.g. `{bar="baz"} | logfmt`) and ad hoc filters on labels not
already in the stream selector, `addLabelToQuery()` falls into the
`addFilterAsLabelFilter()` path (`modifyQuery.ts:185-195, 511-540`) which has no
deduplication, so the expression now contains a `| label op` pipeline segment for each ad
hoc filter.
4. Later in the same request, shard splitting creates per-shard sub-requests in
`splitQueriesByStreamShard()` (`shardQuerySplitting.ts:57-209`). For each group, it
computes `targets` from `groups[group].targets` (which are the already-templated
`queries`) and builds `subRequest = { ...request, targets:
interpolateShardingSelector(targets, shardsToQuery) }` at
`shardQuerySplitting.ts:152-155`. This `subRequest` is passed into
`runSplitQuery(datasource, subRequest, ...)` (`shardQuerySplitting.ts:163`), where
`runSplitQuery()` (`querySplitting.ts:291-300`) *again* maps `request.targets` through
`datasource.applyTemplateVariables(query, request.scopedVars, request.filters)`. Because
`subRequest.filters` is the original `request.filters` and the expressions already contain
ad hoc filters from step 3, this second call adds another identical set of label filters
via `addFilterAsLabelFilter()`. The resulting per-shard queries sent via
`datasource.runQuery(subRequest)` contain duplicated ad hoc pipeline filters (e.g. `... |
job=\`grafana\` | job=\`grafana\``), confirming that sharded queries can have ad hoc
filters applied twice in the current code.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** public/app/plugins/datasource/loki/shardQuerySplitting.ts
**Line:** 52:52
**Comment:**
*Logic Error: Similar to time-based splitting, shard splitting now calls `applyTemplateVariables` with `request.filters` before delegating to `runSplitQuery`, but `runSplitQuery`'s sub-requests are executed via `datasource.runQuery`, which applies `applyTemplateVariables` again with the same filters, resulting in duplicated ad hoc label filters on sharded queries; using filters only at execution time avoids double application while still letting shard lookup see an interpolated selector.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished running the review. |
Contributor
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
Contributor
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Test 4nn
Summary by CodeRabbit
Bug Fixes
$__autoand time intervals are now properly substituted before query execution on a per-query basis, ensuring accurate query processing and results.Tests
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/grafana-coderabbit#4](https://github.com/ai-code-review-evaluation/grafana-coderabbit/pull/4)*CodeAnt-AI Description
Interpolate template variables before query splitting and execution for Loki datasource
What Changed
Impact
✅ Accurate time/windowed queries (e.g., $__auto and $step are substituted before execution)✅ Correct shard label lookups using the full stream selector✅ Consistent per-query substitution before splitting💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.