Grafana / PR #3

Plugins: Renamed instrumentation middleware to metrics middleware

grafana/grafana#76186
TypeScript / Go
Golden recall: 50.0% Extra findings: 3

Golden Comments (1/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 ContextualLoggerMiddleware methods (QueryData, CallResource, CheckHealth, CollectMetrics) panic with a nil request — direct access to req.PluginContext without nil check. Regression vs the old LoggerMiddleware that handled nil requests. HIGH ✓ Found HIGH FriendlyReviewer found the nil-check missing in both middlewares: MetricsMiddleware.QueryData (L161), CallResource (L177), CheckHealth (L186), CollectMetrics (L194) and ContextualLoggerMiddleware.QueryData (L37), CallResource (L42), CheckHealth (L47), CollectMetrics (L52). The golden only mentioned one middleware.
#2 traceID is no longer logged for plugin requests — the tracing import and extraction/add logic were removed from LoggerMiddleware during the refactoring. Impacts debugging and traceability of plugin requests. LOW ✗ Missed FriendlyReviewer did not detect the traceID regression. The loss of traceID is a silent behavioral change that requires comparing the old and new code — FriendlyReviewer reviews the new code alone.

Supplementary Findings (5 findings)

Finding File Severity Legitimate?
Old file not deleted: instrumentation_middleware.go + test still present → duplicate symbols in package metrics_middleware.go:29 HIGH ⚠ Artefact — our clean reproduction missed deleting the renamed file. Not a bug in the original PR.
Fragile type assertion: d.l.FromContext(ctx).(*log.ConcreteLogger) assumes infra always returns *ConcreteLogger logger.go:48 MEDIUM ✓ Real risk — a change in the infra package would silently lose contextual attributes
Ambiguous doc: FromContext uses "context" ambiguously (Go context.Context vs key-value context) ifaces.go:17 LOW ✓ Docstring could be clearer
Outdated test name: TestInstrumentationMiddleware should be TestMetricsMiddleware metrics_middleware_test.go:21 LOW ✓ Reflects the rename
Typo promRegisterer: double 'r' propagated in parameter name pluginsintegration.go:148 LOW ✓ Acceptable, does not block compilation

Honest Analysis

FriendlyReviewer found the most important bug in this PR (golden #1: nil panic in middleware methods) and actually extended it beyond what the golden set described. The golden expected a finding in ContextualLoggerMiddleware only, but FriendlyReviewer flagged the same nil-check missing pattern in both middlewares — MetricsMiddleware and ContextualLoggerMiddleware — across all four handler methods. That is thoroughness: it didn't stop at the first file.

Golden #2 (traceID lost from logs) was missed entirely. This regression is inherently difficult to detect when reviewing only the new code — the tracing import and traceID extraction logic were simply removed from the old LoggerMiddleware during the refactoring, and there is nothing in the new middlewares to suggest something is missing. A before-and-after diff comparison would be needed to catch this, and FriendlyReviewer does not operate that way (yet).

The highest-severity finding in FriendlyReviewer's output ("old file not deleted, duplicate symbols") is technically correct but is a reproduction artefact, not a real PR bug. The clean reproduction process used git checkout on the diff file list, which does not capture the deletion side of a rename — so both the old and new files ended up on the branch. On the actual PR, only the renamed file exists. This finding is excluded from the legitimate count.

The 4 legitimate supplementary findings cover quality and maintenance aspects well: a fragile type assertion that could silently lose context (MEDIUM), an outdated test name (LOW), an ambiguous docstring (LOW), and a propagated typo (LOW). The type assertion finding is the pick of these — FriendlyReviewer identified that d.l.FromContext(ctx).(*log.ConcreteLogger) would silently fall back to a default logger if the infra package changed its return type, which is a real maintenance risk even if it works today.

Overall, FriendlyReviewer caught the critical runtime bug (nil panic) while missing a subtle logging regression. The reproduction artefact finding is noise from the benchmark methodology, not a signal quality issue. The legitimate extra findings are solid, particularly the type assertion risk and the discovery that the nil-check problem existed in both middlewares, not just one.

FriendlyReviewer's full review

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

friendly-reviewer bot

Hi there 👋

🌥️ Tech
3 high, 1 medium, 3 low

The Merge Request introduces contextual logging by adding FromContext(context.Context) Logger to the Logger interface and its implementations, and splits the previous monolithic InstrumentationMiddleware into separate MetricsMiddleware and ContextualLoggerMiddleware. While the architectural changes are sound, the MR is blocked from compilation due to the old instrumentation_middleware.go and its test file not being deleted, causing duplicate symbol definitions in the clientmiddleware package. Additionally, the new middlewares are missing nil-checks for request parameters, creating potential nil-pointer panics. Minor findings include ambiguous documentation in the interface, a fragile type assertion in the production logger, a test function name mismatch, and a propagated spelling mistake.

These might need a close look

  • 🔴 pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go (L29)
    BLOCKING: The old instrumentation_middleware.go still exists in the same package, defining duplicate symbols (pluginMetrics, InstrumentationMiddleware, instrumentContext, etc.). This file must be deleted to avoid compilation errors. Its test file instrumentation_middleware_test.go must also be removed.
  • 🔴 pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go (L161)
    Missing nil-check for req parameter in QueryData. Also missing in CallResource (177), CheckHealth (186), and CollectMetrics (194). The existing LoggerMiddleware guards against nil req; add similar guards to prevent nil-pointer panics.
  • 🔴 pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go (L37)
    Missing nil-check for req parameter in QueryData. Also missing in CallResource (42), CheckHealth (47), and CollectMetrics (52). Add the same nil-guard pattern used by LoggerMiddleware.

Worth checking

  • 🟡 pkg/plugins/log/logger.go (L48)
    The type assertion d.l.FromContext(ctx).(*log.ConcreteLogger) relies on the infra package always returning *ConcreteLogger. If the infra package changes, the fallback to d.New() silently loses contextual attributes. Add a comment explaining the safety or refactor to avoid the type assertion.

Small things (take or leave)

  • 🔵 pkg/plugins/log/ifaces.go (L17)
    The doc comment for FromContext uses "context" ambiguously, which could be confused with the key-value context mentioned in New(). Clarify that it refers to Go's context.Context.
  • 🔵 pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go (L21)
    Test function is named TestInstrumentationMiddleware but tests the new MetricsMiddleware. Rename to TestMetricsMiddleware to match the renamed middleware.
  • 🔵 pkg/services/pluginsintegration/pluginsintegration.go (L148)
    Parameter name promRegisterer is misspelled (double 'r'). Consider fixing for consistency, though it does not affect compilation.