Grafana PR #4 — Live Benchmark — FriendlyReviewer

Golden Comments (0/1 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	`applyTemplateVariables` is called with `request.filters` as 3rd parameter, but this parameter is not used in the corresponding test mock	LOW	✗ Missed	FriendlyReviewer found a related issue: the mock of `replace()` in the test does not match the real `applyTemplateVariables` behavior (`$__auto` excluded from scopedVars in production), but this is a different problem from the omitted `filters` parameter.

Supplementary Findings (10 findings)

Finding	File	Severity	Legitimate?
Double interpolation risk: `runShardSplitQuery()` calls `applyTemplateVariables()`, then passes queries to `runSplitQuery()` which calls it again	querySplitting.ts:296	CRITICAL	✓ Real bug — double call to `applyTemplateVariables`, potential side effects
Double interpolation (same issue): shard splitting layer should not interpolate since `runSplitQuery` already does, or pass a flag to skip re-interpolation	shardQuerySplitting.ts:48	CRITICAL	✓ Same root cause, affects the shard splitting path
Unhandled promise rejection: `groupTargetsByQueryType().then()` without `.catch()`	shardQuerySplitting.ts:223	CRITICAL	✓ Real bug — async error silently lost, subscriber left hanging
Inconsistent error handling: `retry()` called without arguments, bypasses `isRetriableError` check	shardQuerySplitting.ts:228	CRITICAL	✓ Real bug — all errors are retried, even non-retriable ones
Test mock `replace()` does not match real `applyTemplateVariables`: `$__auto` excluded from scopedVars in production	querySplitting.test.ts:77	MEDIUM	✓ Test tests a scenario impossible in production
Assertion fragile: access to `mock.calls[0]` without guard — TypeError if `runQuery` never called	querySplitting.test.ts:85	MEDIUM	✓ Early failure would give misleading error
Test uses real `applyTemplateVariables` instead of a mock — integration test, not unit test	shardQuerySplitting.test.ts:81	MEDIUM	✓ Depends on real datasource implementation
Fragile ordering: `querySupportsSplitting` checked after interpolation — range variables could be incorrectly split	querySplitting.ts:299	LOW	✓ Valid design precaution
Test ignores emitted values in `toEmitValuesWith` — only checks mock calls, not DataQueryResponse	querySplitting.test.ts:84	LOW	✓ Deviates from test patterns elsewhere
Fragile assertion on exact call count (5x) — depends on partitioning internals	shardQuerySplitting.test.ts:112	LOW	✓ Refactoring partitioning would break the test

Honest Analysis

FriendlyReviewer did not find the single golden comment: the unused filters parameter in the test mock for applyTemplateVariables. This is a low-severity test quality issue that requires comparing call signatures between the test and the implementation — a narrow, specific check that the reviewer missed.

However, FriendlyReviewer discovered 4 critical bugs that the golden comment did not capture, all in production code. The double interpolation risk (runShardSplitQuery interpolates then passes to runSplitQuery which interpolates again) appears in two separate files and is a genuine logic error in the pipeline. The unhandled promise rejection (.then() without .catch()) means an async error would leave the Observable subscriber hanging forever. And the retry() call without arguments bypasses the isRetriableError check entirely, retrying even fatal errors. These four bugs are functional issues in the query splitting pipeline, potentially more impactful than the test mock discrepancy described by the golden comment.

The 3 medium findings all relate to test quality — mocks that don't reflect production behavior, fragile assertions without guards, and tests that blur the line between unit and integration testing. These are exactly the kind of maintainability issues a systematic reviewer catches by reading the full test file rather than just the diff.

The 3 low findings are minor: ordering fragility, missing assertions on emitted values, and a brittle call-count assertion. These are acceptable noise — the trade-off for catching the 3 critical bugs.

For a PR of only 4 files and 48 lines of additions, 10 findings (4 critical, 3 medium, 3 low) represents deep coverage. The reviewer spent more effort on the production code pipeline than on the single golden issue, and the result is a much more valuable review than the benchmark reference would suggest.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
4 high, 3 medium, 3 low

This MR introduces template variable interpolation via applyTemplateVariables() in both runSplitQuery() and runShardSplitQuery() to resolve variables like $__auto before splitting queries. While the core functionality is correct, several issues were identified: a double interpolation risk when shard splitting is enabled, inaccurate test mocks that do not mirror production behavior, fragile test assertions tightly coupled to implementation details, and error handling gaps (unhandled promise rejection and bypassed retriability check). These need to be addressed to ensure correctness and maintainability.

These might need a close look

🔴 public/app/plugins/datasource/loki/querySplitting.ts (L296)
Double interpolation risk: When shard splitting is enabled, runShardSplitQuery() calls applyTemplateVariables() and then passes queries to runSplitQuery(), which calls it again. This is redundant and could cause subtle issues if interpolation has side effects. Consider deduplicating or documenting the assumption.
🔴 public/app/plugins/datasource/loki/shardQuerySplitting.ts (L48)
Double interpolation (same issue as querySplitting.ts line 296). The shard splitting layer should not interpolate variables since runSplitQuery already does it, or pass a flag to skip re-interpolation.
🔴 public/app/plugins/datasource/loki/shardQuerySplitting.ts (L223)
Unhandled promise rejection: groupTargetsByQueryType().then() lacks a .catch() handler. A rejected promise would leave the subscriber hanging. Add .catch() that calls subscriber.error().
🔴 public/app/plugins/datasource/loki/shardQuerySplitting.ts (L228)
Inconsistent error handling: retry() is called without arguments in the Observable error callback, bypassing the isRetriableError check. Pass the error to retry() so retriability is properly evaluated.

Worth checking

🟡 public/app/plugins/datasource/loki/querySplitting.test.ts (L77)
Test mock for replace() does not match real applyTemplateVariables behavior: the real implementation excludes $__auto from scopedVars, so the test tests a scenario that cannot occur in production. Use a user-defined variable like $myvar instead.
🟡 public/app/plugins/datasource/loki/querySplitting.test.ts (L85)
Assertion accesses mock.calls[0] without prior check. If runQuery is never called, this throws a TypeError. Add expect(datasource.runQuery).toHaveBeenCalled() before accessing calls.
🟡 public/app/plugins/datasource/loki/shardQuerySplitting.test.ts (L81)
Test uses real applyTemplateVariables instead of a mock, making it an integration test. Mock the method to isolate shard splitting logic.

Small things (take or leave)

🔵 public/app/plugins/datasource/loki/querySplitting.ts (L299)
Fragile ordering: querySupportsSplitting is checked after applyTemplateVariables. If future changes resolve $__range, queries with range variables would be incorrectly split. Consider moving the split check before interpolation.
🔵 public/app/plugins/datasource/loki/querySplitting.test.ts (L84)
Test ignores emitted values in toEmitValuesWith callback and only checks mock call arguments. This deviates from test patterns elsewhere. Add assertions on the DataQueryResponse shape.
🔵 public/app/plugins/datasource/loki/shardQuerySplitting.test.ts (L112)
Fragile assertion on exact call count (5 times). This depends on partitioning details. Use toBeGreaterThan(1) or assert on specific arguments instead.

Advanced Query Processing Architecture