Advanced Query Processing Architecture #7
Conversation
…7534) * Loki query splitting: interpolate queries before execution * Update tests * Prettier * shardQuerySplitting: remove unnecessary call
|
@coderabbitai review |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe Loki datasource template variable interpolation mechanism has been refactored from a batch operation to per-query application. The deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SplittingLogic as Query Splitting Logic
participant DS as LokiDatasource
participant Shard as Shard Processor
Client->>SplittingLogic: request with targets
activate SplittingLogic
SplittingLogic->>SplittingLogic: filter targets (expr, not hidden)
loop For each target query
SplittingLogic->>DS: applyTemplateVariables(query, scopedVars, filters)
DS-->>SplittingLogic: query with interpolated vars
end
SplittingLogic->>Shard: splitQueriesByStreamShard(prepared queries)
Shard-->>SplittingLogic: split queries
SplittingLogic-->>Client: results
deactivate SplittingLogic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
public/app/plugins/datasource/loki/shardQuerySplitting.ts (1)
49-52: LGTM! Per-query template variable application implemented correctly.The refactoring correctly applies template variables per-query with scopedVars and filters. The logic is sound and aligns with the updated interpolation mechanism.
Minor: The filter order here (
query.exprthen!query.hide) differs fromquerySplitting.tsline 296-299 (!query.hidethenquery.expr). While functionally equivalent, consider standardizing the order for consistency:const queries = request.targets - .filter((query) => query.expr) .filter((query) => !query.hide) + .filter((query) => query.expr) .map((query) => datasource.applyTemplateVariables(query, request.scopedVars, request.filters));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
public/app/plugins/datasource/loki/querySplitting.test.ts(1 hunks)public/app/plugins/datasource/loki/querySplitting.ts(1 hunks)public/app/plugins/datasource/loki/shardQuerySplitting.test.ts(4 hunks)public/app/plugins/datasource/loki/shardQuerySplitting.ts(1 hunks)
🔇 Additional comments (5)
public/app/plugins/datasource/loki/querySplitting.test.ts (1)
75-88: LGTM! Test correctly validates per-query template variable interpolation.The new test properly verifies that template variables (
$__autoand$step) are interpolated before query execution. The test setup with the mockedreplacefunction and the assertions checking the interpolated values in the firstrunQuerycall are correct.public/app/plugins/datasource/loki/shardQuerySplitting.test.ts (4)
56-59: LGTM! Mock correctly updated for per-query template variable application.The mock properly implements the new
applyTemplateVariablesAPI by replacing$SELECTORwith{a="b"}on individual queries. The mutation and return pattern aligns with the expected behavior.
84-101: LGTM! Test correctly validates template variable interpolation in shard splitting.The test properly verifies that template variables (
$__autoand$step) are interpolated before execution in the shard splitting path. The assertions correctly check that the interpolated values appear in the query expression with the shard selector modification and in the step parameter.
112-112: LGTM! Correct expectation for per-query template variable application.The expectation of 5 calls to
applyTemplateVariablesaligns with the per-query interpolation approach. This verifies that each query is processed individually rather than in batch.
173-176: LGTM! Mock correctly handles complex selector replacement.The mock properly replaces
$SELECTORwith a more complex selector containing multiple labels, demonstrating that the per-query template variable application works correctly with various selector patterns.
| 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.
🧩 Analysis chain
LGTM! Per-query template variable application implemented correctly.
The refactoring from batch interpolation to per-query template variable application is correct. The filtering logic properly removes hidden queries and those without expressions before applying template variables with the appropriate scopedVars and filters context.
However, note a minor filter ordering inconsistency with shardQuerySplitting.ts (line 49-52), which filters query.expr before !query.hide. While functionally equivalent, consider aligning the filter order for consistency:
🏁 Script executed:
#!/bin/bash
# Verify filter ordering in both splitting implementations
rg -n -A2 'request\.targets' public/app/plugins/datasource/loki/querySplitting.ts public/app/plugins/datasource/loki/shardQuerySplitting.tsLength of output: 1104
Align filter order with shardQuerySplitting.ts for consistency.
The per-query template variable application is correct, but the filter order differs from the sibling implementation in shardQuerySplitting.ts (lines 49-51). While functionally equivalent, reorder to match:
.filter((query) => query.expr)
.filter((query) => !query.hide)🤖 Prompt for AI Agents
In public/app/plugins/datasource/loki/querySplitting.ts around lines 296 to 299
the two .filter calls are ordered differently than shardQuerySplitting.ts;
reorder the filters to first filter for query.expr then for !query.hide so the
chain becomes .filter((query) => query.expr).filter((query) => !query.hide)
before mapping, matching the sibling implementation for consistency.
Test 4
Summary by CodeRabbit
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/grafana-coderabbit#4