Grafana / PR #7

Database Performance Optimizations

grafana/grafana#80329 TypeScript / Go
Golden recall: 100.0% Extra findings: 16

Golden Comments (1/1 found)

# Comment Severity Status FriendlyReviewer Detail
#1 r.log.Error(...) used for debug/info messages in annotation cleanup code → pollutes production error logs with routine informational messages HIGH ✓ Found Detected across 6 locations in xorm_store.go (L537, L538, L555, L558, L580, L583). Categorized as "Incorrect log level: log.Error used for informational cleanup status." Also identified the companion issue: logging full annotation ID lists.

Supplementary Findings (21 findings)

Additional issues found by FriendlyReviewer beyond the golden comment set.

Finding File Severity
Logging full annotation ID lists (up to 32,767 IDs) → severe performance degradation, log storage bloat, data exposure xorm_store.go HIGH
Ticker resource leak: time.NewTicker never calls ticker.Stop() → ticker runs indefinitely cleanup.go:79 HIGH
Reducing cleanup interval from 10m to 1m with 9m timeout → up to 9 overlapping cleanup cycles, risking MySQL deadlocks (contradicts the annotation refactoring purpose) cleanup.go:79 HIGH
ServerLockService injected but never used → every HA instance runs cleanup independently every 1 minute with no coordination cleanup.go:55 HIGH
Redundant temp file cleanup: cleanUpTmpFiles called both at startup and inside clean() → duplicate work per tick cleanup.go:147 HIGH
Non-idiomatic Go variable naming: x, y instead of affected, err across result variables xorm_store.go:539,559,584 MEDIUM
CleanAnnotations does not clean annotation_tag entries → relies entirely on caller, risking orphaned tags xorm_store.go:563 MEDIUM
Test TestIntegrationOldAnnotationsAreDeletedFirst does not verify annotation_tag cleanup → orphaned tags go undetected cleanup_test.go:186 MEDIUM
Large-batch test may not exercise SQLite path; ensure test DB is SQLite or add separate test cleanup_test.go:163 MEDIUM
Unused dependency store (db.DB) injected into CleanupService → dead code removing clarity cleanup.go:37 MEDIUM
Inappropriate log level: logger.Error for cancelled context → should be logger.Warn or logger.Info cleanup.go:89 MEDIUM
9-minute timeout for clean() but only cleanUpOldAnnotations checks for deadline errors → other jobs may ignore cancellation cleanup.go:105 MEDIUM
SQL injection risk (low severity): IDs interpolated via fmt.Sprintf in SQLite fallback path → use parameterized queries with sub-batching xorm_store.go:606 LOW
Redundant error logging after nil guard → err field is always nil when log line executes xorm_store.go:538,558 LOW
Hardcoded row limits (200k, 150k) in deleteStaleQueryHistory → should be configurable via settings cleanup.go:252 LOW
Hardcoded IDs in createTestAnnotations may conflict with auto-increment → use auto-generated IDs cleanup_test.go:236 LOW
Test distribution pattern (i%3) undocumented → intent unclear to future maintainers cleanup_test.go:96 LOW
Context shadowing in job loop → per-job context uses same variable name as parent scope cleanup.go:101 LOW

Honest Analysis

This is a strong result for FriendlyReviewer. The single golden comment — log level misuse spanning 6 locations in xorm_store.go — was correctly identified. Beyond that, FriendlyReviewer surfaced 5 additional high-severity issues the human reviewer didn't flag: the ticker resource leak, the dangerous interval/timeout mismatch causing overlapping cleanup cycles, the unused HA coordination service, the redundant temp file cleanup, and the logging of full annotation ID lists creating performance and data exposure risks.

What stands out is the breadth of coverage: FriendlyReviewer found 21 supplementary issues spanning 3 files (xorm_store.go, cleanup.go, cleanup_test.go), including 5 additional high-severity issues that the golden comment set didn't cover. The ticker resource leak (ticker.Stop() never called) and the interval reduction creating overlapping cleanup cycles are genuine production risks — the annotation refactoring was specifically done to avoid MySQL deadlocks, and the faster ticker directly undermines that fix.

The medium and low findings cover code quality concerns (variable naming, test completeness, dead dependencies, log level appropriateness) and security (SQL injection risk in SQLite fallback). The test coverage gaps are particularly useful — the large-batch test may not actually exercise the SQLite path it's supposed to test, and the old-annotations test doesn't validate tag cleanup.

This benchmark shows FriendlyReviewer operating with full coverage across all modified files, delivering a comprehensive review that goes well beyond the human-identified golden comments. The 100% golden recall paired with 21 supplementary findings demonstrates the value of automated review as a complement — catching not just the known issues but uncovering systemic problems the human reviewer didn't flag.

FriendlyReviewer's full review

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

friendlyreviewer-staging bot commented now

Hi there 👋

🌥️ Tech
9 high, 8 medium, 5 low

The MR introduces two main changes: (1) refactoring annotation cleanup to use a two-phase (fetch IDs, then delete by IDs) batch processing approach to avoid MySQL deadlocks, and (2) reducing the cleanup ticker interval from 10 minutes to 1 minute. While the architectural direction is sound, the review identified several systemic issues that must be addressed before merging.

Critical issues:

  • Log level misuse: log.Error is used extensively for routine informational messages (cleanup status, successful operations). This will pollute production monitoring and trigger false alarms. All such instances should use log.Info or log.Debug.
  • Logging full ID lists: The code logs every annotation ID in large batches (up to 32,767 IDs). This causes severe performance degradation, log storage bloat, and potential data exposure. Log only the count or a sample.
  • Ticker resource leak: The new time.NewTicker in cleanup.go never calls ticker.Stop(). Add defer ticker.Stop().
  • Unused dependencies: ServerLockService and store are injected but never used. In HA setups, this means cleanup jobs may execute concurrently without coordination. Either remove them or properly use LockAndExecute.
  • Redundant temp file cleanup: cleanUpTmpFiles is called both at startup and inside clean(), causing duplicate work per tick.

Medium issues:

  • Non-idiomatic variable naming (x, y instead of affected, err).
  • SQL injection risk (low severity) in SQLite fallback path where IDs are interpolated into SQL.
  • Test gaps: large-batch test doesn't actually target SQLite; old-annotations test doesn't validate tag cleanup.
  • Context shadowing in job loop.
  • Potential back-to-back cleanups if jobs exceed the new 1-minute interval.

Minor issues:

  • Redundant error logging after nil guard.
  • Hardcoded row limits in deleteStaleQueryHistory should be configurable.

These might need a close look

  • 🔴 pkg/services/annotations/annotationsimpl/xorm_store.go (L537) — Incorrect log level: log.Error used for informational cleanup status. Use log.Info or log.Debug. Also logging full IDs list — log only count or sample.
  • 🔴 pkg/services/annotations/annotationsimpl/xorm_store.go (L538) — Incorrect log level for successful deletion result. Also logs err which is guaranteed nil after guard. Use log.Info and remove the redundant err field.
  • 🔴 pkg/services/annotations/annotationsimpl/xorm_store.go (L555) — Incorrect log level and logging full IDs list. Same pattern as line 537.
  • 🔴 pkg/services/annotations/annotationsimpl/xorm_store.go (L558) — Incorrect log level and redundant nil err field. Use log.Info.
  • 🔴 pkg/services/annotations/annotationsimpl/xorm_store.go (L580) — Incorrect log level for orphaned tag cleanup. Logging full IDs list. Use log.Info or log.Debug.
  • 🔴 pkg/services/annotations/annotationsimpl/xorm_store.go (L583) — Incorrect log level for successful orphaned tag deletion. Use log.Info.
  • 🔴 pkg/services/cleanup/cleanup.go (L79) — Ticker resource leak: ticker.Stop() never called. Add defer ticker.Stop().
  • 🔴 pkg/services/cleanup/cleanup.go (L79) — Reducing interval from 10m to 1m may cause back-to-back executions if cleanup takes >1m. Add guard or use timer with reset.
  • 🔴 pkg/services/cleanup/cleanup.go (L55) — Unused dependency ServerLockService; in HA setups concurrent cleanups will occur. Either remove or use LockAndExecute.

Worth checking

  • 🟡 pkg/services/annotations/annotationsimpl/xorm_store.go (L539) — Non-idiomatic Go variable naming: x, y instead of affected, err. Same issue on lines 559, 584.
  • 🟡 pkg/services/annotations/annotationsimpl/xorm_store.go (L563) — CleanAnnotations does not clean annotation_tag entries, relying entirely on caller. Document or cleanup inside function.
  • 🟡 pkg/services/annotations/annotationsimpl/cleanup_test.go (L186) — Test TestIntegrationOldAnnotationsAreDeletedFirst does not verify annotation_tag cleanup, leaving orphaned tags.
  • 🟡 pkg/services/annotations/annotationsimpl/cleanup_test.go (L163) — Large batch test may not actually exercise SQLite path; ensure test DB is SQLite or add separate test.
  • 🟡 pkg/services/cleanup/cleanup.go (L37) — Unused dependency store (db.DB). Remove to avoid confusion.
  • 🟡 pkg/services/cleanup/cleanup.go (L89) — Inappropriate log level: logger.Error for cancelled context. Use logger.Warn or logger.Info.
  • 🟡 pkg/services/cleanup/cleanup.go (L147) — Redundant execution: cleanUpTmpFiles called both at startup and inside clean(). Remove one.
  • 🟡 pkg/services/cleanup/cleanup.go (L105) — 9-minute timeout for clean() but only cleanUpOldAnnotations checks for deadline errors; other jobs may ignore cancellation.

Small things (take or leave)

  • 🔵 pkg/services/annotations/annotationsimpl/xorm_store.go (L606) — SQL injection risk (low): IDs interpolated via fmt.Sprintf in SQLite fallback. Use parameterized queries with sub-batching.
  • 🔵 pkg/services/annotations/annotationsimpl/cleanup_test.go (L236) — Hardcoded IDs in createTestAnnotations may conflict with auto-increment; use auto-generated IDs.
  • 🔵 pkg/services/annotations/annotationsimpl/cleanup_test.go (L96) — Test distribution pattern (i%3) should be documented.
  • 🔵 pkg/services/cleanup/cleanup.go (L101) — Context shadowing in loop; use separate variable for per-job context.
  • 🔵 pkg/services/cleanup/cleanup.go (L252) — Hardcoded row limits (200k, 150k) in deleteStaleQueryHistory; should be configurable.

Review time: 11m 45s