Advanced Query Processing Architecture#5
Conversation
…7534) * Loki query splitting: interpolate queries before execution * Update tests * Prettier * shardQuerySplitting: remove unnecessary call
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes replace batch-level query template variable substitution with per-query interpolation in Loki's query splitting logic. The datasource now applies template variables individually to each query before execution, changing from a single batch operation to a mapped per-query approach. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (3)
public/app/plugins/datasource/loki/shardQuerySplitting.ts (1)
49-52: Filter order inconsistency withquerySplitting.ts.The filter order here is
.filter(expr)then.filter(!hide), but inquerySplitting.ts(lines 296-298) the order is reversed:.filter(!hide)then.filter(expr). While both achieve the same end result, maintaining consistent filter ordering across the codebase improves readability and reduces cognitive overhead.Suggested fix to align filter order with querySplitting.ts
export function runShardSplitQuery(datasource: LokiDatasource, request: DataQueryRequest<LokiQuery>) { 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)); return splitQueriesByStreamShard(datasource, request, queries); }public/app/plugins/datasource/loki/shardQuerySplitting.test.ts (2)
56-59: Mock mutates input instead of returning a new object.The mock implementation mutates the input
queryobject directly. While this works for the test, productionapplyTemplateVariablestypically returns a new object to avoid side effects. Consider returning a new object to better simulate production behavior.Suggested improvement
datasource.applyTemplateVariables = jest.fn().mockImplementation((query: LokiQuery) => { - query.expr = query.expr.replace('$SELECTOR', '{a="b"}'); - return query; + return { + ...query, + expr: query.expr.replace('$SELECTOR', '{a="b"}'), + }; });
173-176: Same mutation pattern as the earlier mock.This mock also mutates the input object directly. Consider using the same immutable pattern suggested earlier for consistency.
Suggested improvement
datasource.applyTemplateVariables = jest.fn().mockImplementation((query: LokiQuery) => { - query.expr = query.expr.replace('$SELECTOR', '{service_name="test", filter="true"}'); - return query; + return { + ...query, + expr: query.expr.replace('$SELECTOR', '{service_name="test", filter="true"}'), + }; });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
public/app/plugins/datasource/loki/querySplitting.test.tspublic/app/plugins/datasource/loki/querySplitting.tspublic/app/plugins/datasource/loki/shardQuerySplitting.test.tspublic/app/plugins/datasource/loki/shardQuerySplitting.ts
🧰 Additional context used
🧬 Code graph analysis (3)
public/app/plugins/datasource/loki/querySplitting.ts (1)
public/app/plugins/datasource/loki/datasource.ts (1)
query(299-344)
public/app/plugins/datasource/loki/querySplitting.test.ts (1)
public/app/plugins/datasource/loki/querySplitting.ts (1)
runSplitQuery(291-357)
public/app/plugins/datasource/loki/shardQuerySplitting.test.ts (2)
public/app/plugins/datasource/loki/types.ts (1)
LokiQuery(24-33)public/app/plugins/datasource/loki/shardQuerySplitting.ts (1)
runShardSplitQuery(48-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Postgres (7/8)
🔇 Additional comments (4)
public/app/plugins/datasource/loki/querySplitting.test.ts (1)
75-88: LGTM!The test correctly verifies that template variable interpolation occurs before query execution. It properly mocks the template service's
replacefunction and validates that bothexprandstepfields contain interpolated values in the firstrunQuerycall.public/app/plugins/datasource/loki/querySplitting.ts (1)
296-299: LGTM!The per-query interpolation approach correctly applies template variables to each individual query after filtering out hidden and empty queries. This aligns with the PR objective and ensures variables are resolved before query processing.
public/app/plugins/datasource/loki/shardQuerySplitting.test.ts (2)
84-101: LGTM!The test correctly verifies that template variable interpolation occurs before query execution in the shard splitting path. It validates that both the expression (with shard selector added) and step values are properly interpolated.
110-112: The expected call count of 5 is correct.
applyTemplateVariablesis called once initially inrunShardSplitQuery(line 52), then called 4 additional times insiderunSplitQuery(line 299 inquerySplitting.ts)—once for each of the 4 groups created during shard cycling (1 empty group + 3 shard cycles from the 5 shards).
|
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! |
|
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! |
2 participants
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)*