Grafana / PR #23

Advanced Query Processing Architecture

grafana/grafana#107534
Golden recall: 0% Extra findings: 6

Golden Comments (0/1 found)

# Comment Severity Status FriendlyReviewer Detail
#1 The applyTemplateVariables method is called with request.filters as the third parameter, but this parameter is not used in the corresponding test setup. LOW ✗ Not found The review covers the PR's tests well — the $__auto test asserting a mock artifact, the shard test freezing the double interpolation, untested fallback paths — but not this detail: the third parameter request.filters passed to applyTemplateVariables is not used in the test setup. See public/app/plugins/datasource/loki/querySplitting.test.ts

Supplementary Findings (6 findings)

Finding File Severity Legitimate?
Adhoc filters applied twice in the shard path: applyTemplateVariables runs in runShardSplitQuery and again inside runSplitQuery per shard group, and addAdHocFilters is not idempotent — filters duplicated in the final selector shardQuerySplitting.ts:52 HIGH ✓ Genuine functional bug found by FR — the double interpolation — not covered by the dataset
$__auto test asserts a mock artifact: applyTemplateVariables leaves $__auto unresolved for backend interpolation, so the test gives false confidence querySplitting.test.ts:88 MEDIUM ✓ Real misleading test assertion about genuine frontend-resolved variables
Shard test asserts applyTemplateVariables called 5 times, locking in the double-application implementation detail shardQuerySplitting.test.ts:112 MEDIUM ✓ Test enshrines the bug; asserting the double interpolation as expected behavior
New test does not verify interpolation's interaction with grouping and non-splittable classification querySplitting.test.ts:75 LOW ✓ Coverage gap: interpolated step in grouping, $__range non-splittable, ordering vs partition
Misleading comment: "No more than 50% of the remaining shards" while constrainGroupSize caps at 0.7 (70%) shardQuerySplitting.test.ts:469 LOW ✓ Comment/code drift; cosmetic
No-shard and fetch-error fallback paths untested shardQuerySplitting.test.ts:53 LOW ✓ Coverage gap: single unsharded runSplitQuery completing with LoadingState.Done is unverified

Honest Analysis

FriendlyReviewer found 0 of the 1 golden comment of this PR — recall 0%. The golden is a small test-setup detail: applyTemplateVariables is called with request.filters as its third parameter, but the corresponding test setup never uses that parameter (querySplitting.test.ts). The review scrutinized the PR's tests — the $__auto test asserting a mock artifact, the shard test freezing the double interpolation, untested fallback paths — yet missed this specific unused argument.

The score, however, understates what the review found. FriendlyReviewer identified a genuine functional bug that the dataset does not cover: in the shard path, applyTemplateVariables is invoked both in runShardSplitQuery and again inside runSplitQuery for each shard group, and since addAdHocFilters is not idempotent, adhoc filters end up duplicated in the final selector (shardQuerySplitting.ts L52, flagged 🔴). That is a real regression in query results — arguably more consequential than the dataset's single test-setup comment — and it is unscored.

This is the clearest case in the run of the recall metric not telling the whole story: the review's best finding is invisible to the score. The flip side is equally true: the only dataset-credited comment is a test-hygiene detail that the review missed despite deep test scrutiny, so the zero is also earned. Both facts matter when reading the result.

Overall: the review understood the PR's mechanics correctly and caught its most user-visible defect, but the scored outcome remains 0% — a reminder that recall measures overlap with a specific reference set, not the total value of a review.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌤️ Feat
1 high, 2 medium, 3 low 1 mitigated

The MR correctly moves query interpolation to the start of the query-splitting pipeline for both time-based and shard-based paths, using applyTemplateVariables instead of the previous per-step interpolation, which fixes variable resolution (e.g. $env) and aligns metric step grouping with the resolved step. However, the shard path now double-applies template variables/adhoc filters: applyTemplateVariables is invoked both in runShardSplitQuery and again inside runSplitQuery for each shard group, and since addAdHocFilters is not idempotent, adhoc filters are duplicated in the final selector. Tests also have gaps/misleading assertions: the $__auto test asserts a mock artifact rather than real applyTemplateVariables behavior, the shard test locks in the double-interpolation implementation detail, and fallback/coverage paths are untested. These issues should be addressed before merging.

These might need a close look

  • 🔴 public/app/plugins/datasource/loki/shardQuerySplitting.ts (L52)
    Adhoc filters are applied twice in the shard path. applyTemplateVariables is called here and again inside runSplitQuery (invoked for each shard group), and addAdHocFilters is not idempotent, so adhoc filters are duplicated in the final selector. Interpolate exactly once.

Worth checking

  • 🟡 public/app/plugins/datasource/loki/querySplitting.test.ts (L88)
    Test $__auto expectation does not reflect real applyTemplateVariables behavior: applyTemplateVariables leaves $__auto unresolved for backend interpolation, so the test asserts a mock artifact and gives false confidence. Use a genuinely frontend-resolved variable and assert $__auto/$__range remain unresolved.
  • 🟡 public/app/plugins/datasource/loki/shardQuerySplitting.test.ts (L112)
    Test asserts applyTemplateVariables called 5 times (double interpolation), locking in the double-application bug. Assert final expr/step and that filters are applied exactly once.
Small things (take or leave)
  • 🔵 public/app/plugins/datasource/loki/querySplitting.test.ts (L75)
    New test does not cover interpolation's interaction with grouping and non-splittable classification: does not verify interpolated step participates in metric grouping, that $__range queries remain non-splittable, or that interpolation happens before partitionTimeRange/direction classification.
  • 🔵 public/app/plugins/datasource/loki/shardQuerySplitting.test.ts (L469)
    Misleading comment says 'No more than 50% of the remaining shards' but constrainGroupSize uses 0.7 (70%) cap. Comments should say 70%.
  • 🔵 public/app/plugins/datasource/loki/shardQuerySplitting.test.ts (L53)
    Coverage gap: no-shard and fetch-error fallback paths are untested. Add test asserting a single unsharded runSplitQuery completes with LoadingState.Done.

✅ Feature-level checklist

Partially covered

  • ⚠️ Interpolate queries at the start of the process (fixes #107530)
    Interpolation is moved to the start via applyTemplateVariables, but in the shard path interpolation is applied a second time, causing adhoc filters to be duplicated. Tests also do not accurately assert interpolation of genuine frontend-resolved variables.