Grafana PR #22 — Live Benchmark

Golden Comments (0/2 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	The `ContextualLoggerMiddleware` methods (QueryData, CallResource, CheckHealth, CollectMetrics) panic when a nil request is received. This occurs because they directly access `req.PluginContext` (via the `instrumentContext` function) without first checking if `req` is nil. This is a regression, as previous middleware layers gracefully handled nil requests.	HIGH	✗ Not found	The review (1 medium, 5 low) covers the rename itself — residual references to the old name, a copied doc comment, an ordering dependency — and asserts "no functional regressions". It does not detect the panic path: `instrumentContext` accesses `req.PluginContext` without a nil check, where previous middleware layers gracefully handled nil requests. See `pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go`
#2	The traceID is no longer logged for plugin requests. During a refactoring, the tracing import and the logic to extract and add traceID from the context to log parameters were removed from the `LoggerMiddleware`. The newly introduced `ContextualLoggerMiddleware` does not add this information, resulting in missing traceID in plugin request logs.	LOW	✗ Not found	No candidate mentions the missing traceID. The findings cover the disconnected test logger, the dead fallback branch, the middleware ordering and the residual names — not the removal of the tracing import and the traceID extraction logic from `LoggerMiddleware`. See `pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go`

Supplementary Findings (6 findings)

Finding	File	Severity	Legitimate?
`TestLogger.FromContext` returns a fresh disconnected `TestLogger` — tests asserting on `mw.logger.InfoLogs` silently see zero calls	fake.go:46	MEDIUM	✓ Real testability trap that silently weakens test assertions
`logRequest` now relies on `ContextualLoggerMiddleware` unconditionally preceding `LoggerMiddleware` — fields would silently vanish if reordered	logger_middleware.go:58	LOW	✓ Legitimate implicit ordering dependency; non-blocking given current wiring
Dead fallback branch in `grafanaInfraLogWrapper.FromContext` — the type assertion can never fail	logger.go:48	LOW	✓ Dead code; borderline, defensive code is sometimes intentional
Test function still named `TestInstrumentationMiddleware` — the only remaining `Instrumentation*` reference in the package	metrics_middleware_test.go:21	LOW	✓ Cosmetic but real leftover of the rename
Logger still created with the stale `"plugin.instrumentation"` scope name	pluginsintegration.go:161	LOW	✓ Cosmetic leftover of the rename; runtime `FromContext` path makes it largely irrelevant
Copy-pasted doc comment on `FromContext` that is semantically muddled	ifaces.go:22	LOW	✓ Borderline: documentation nit only

Honest Analysis

FriendlyReviewer found 0 of the 2 golden comments of this PR — recall 0%. The review validated the rename end-to-end and concluded that it found "no functional regressions" — yet both goldens target regressions. Golden #1 (HIGH) is a panic: the ContextualLoggerMiddleware methods access req.PluginContext via instrumentContext without a nil check, where the earlier middleware layers gracefully handled nil requests. Golden #2 (LOW) is a functional loss: the tracing import and the traceID extraction logic were removed during the refactoring, so plugin request logs no longer carry a traceID.

The six findings that the review did produce (1 medium, 5 low) are all about rename hygiene and test robustness: a disconnected test logger, a dead fallback branch, an implicit ordering dependency, stale naming, and a copy-pasted doc comment. Nothing touches runtime behavior of the moved logger — which is precisely where both goldens sit. For a "chore" PR, the review treated the change as a mechanical rename and never asked whether the relocated middleware still behaves as before.

The most damaging aspect is the review's own strongest assertion: "no functional regressions." The dataset contradicts that claim with a guaranteed panic path on nil requests and a lost traceID in plugin logs. The supplementary findings are individually legitimate — the ordering dependency (logger_middleware.go L58) is the most substantive, since reordering would silently drop log fields — but they are all minor, and none of them would prevent a merge that ships a crash.

Overall this is the weakest result of the run: a review that certifies a rename as safe while the change introduces a crash regression and drops observability data. The verdict was correct on the mechanics of the rename and wrong on everything that could go wrong with it.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

⛅ Tech	🌤️ Feat
1 medium, 5 low	1 ok, 2 mitigated

The MR cleanly accomplishes both stated goals: the InstrumentationMiddleware has been renamed to MetricsMiddleware (file, package, constructor and wiring all updated, with no remaining NewInstrumentationMiddleware calls), and the contextual-logger concern has been moved out of the metrics middleware into LoggerMiddleware, which now obtains plugin context via Logger.FromContext(ctx). The middleware ordering in pluginsintegration.go (Tracing → Metrics → ContextualLogger → Logger) is correct, and the new metrics tests are well-structured (fresh registry per subtest, correct endpoint/status/size assertions).

The review found no functional regressions — the requested rename and logger relocation behave as intended. However, several small consistency and robustness issues remain: two stale references to the old Instrumentation name survive (a test function name and the base logger scope), the fake TestLogger.FromContext returns a disconnected logger that can silently break test assertions on log output, the real FromContext contains a dead fallback branch from a guaranteed-succeeding type assertion, LoggerMiddleware now has an implicit, ordering-sensitive dependency on ContextualLoggerMiddleware that would silently drop fields if reordered, and a copy-pasted doc comment on the interface. None of these are blocking, but they are worth tidying up.

Worth checking

🟡 pkg/plugins/log/fake.go (L46)
TestLogger.FromContext returns a brand-new disconnected TestLogger, discarding the receiver. The real implementation shares the underlying sink with the receiver, so this divergence is a silent testability trap: with the middleware now logging via m.logger.FromContext(ctx).Info(...), tests that set mw.logger = log.NewTestLogger() and then assert on mw.logger.InfoLogs will see zero calls because the log goes to the fresh, unreferenced instance. Prefer returning f itself (mirroring real semantics) or documenting that a fresh instance is intentional.

Small things (take or leave)

🔵 pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go (L58)
logRequest now relies entirely on m.logger.FromContext(ctx) for endpoint/pluginId/dsName/dsUID/uname instead of receiving pluginCtx/endpoint directly. This works today only because ContextualLoggerMiddleware unconditionally precedes LoggerMiddleware; if the two are ever reordered or the contextual middleware is removed while PluginLogBackendRequests stays enabled, those fields would silently vanish from the "Plugin Request Completed" line with no warning. Consider an integration test or documented ordering requirement (not blocking given current wiring).
🔵 pkg/plugins/log/logger.go (L48)
In grafanaInfraLogWrapper.FromContext, the type assertion d.l.FromContext(ctx).(*log.ConcreteLogger) can never fail because d.l is a *log.ConcreteLogger and ConcreteLogger.FromContext always returns a *ConcreteLogger. The if !ok { return d.New() } fallback is dead code and misleading; simplify to a direct assertion, or keep it only with documentation if defense against a future interface return type is desired.
🔵 pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go (L21)
The test function is still named TestInstrumentationMiddleware, referencing the pre-rename symbol that no longer exists. This is the only remaining Instrumentation* reference in the package. Rename it to TestMetricsMiddleware for consistency with the rename.
🔵 pkg/services/pluginsintegration/pluginsintegration.go (L161)
clientmiddleware.NewLoggerMiddleware(cfg, log.New("plugin.instrumentation")) still uses the logger scope/name "plugin.instrumentation", a leftover from the pre-rename instrumentation era. Given the rename intent, rename it to something like "plugin.metrics" or "plugin.request-log". Cosmetic and non-blocking since the runtime FromContext path makes the base name largely irrelevant to output.
🔵 pkg/plugins/log/ifaces.go (L22)
The doc comment // FromContext returns a new contextual Logger that has this logger's context plus the given context. is a verbatim copy of the New comment and is semantically muddled for FromContext, which derives context from a context.Context rather than variadic key/value args. Clarify it, e.g. "returns a Logger carrying this logger's context plus the attributes registered in the given context.Context".

✅ Feature-level checklist

Looks good

✅ Move the contextual logger from the Instrumentation/Metrics middleware to the LoggerMiddleware.
Verified end-to-end: the contextual-logger middleware stores attributes via log.WithContextualAttributes, LoggerMiddleware reads them back through m.logger.FromContext(ctx).Info(...), and MetricsMiddleware contains no logger logic. Middleware ordering in pluginsintegration.go guarantees the contextual logger is available when LoggerMiddleware logs.

Partially covered

⚠️ Rename the InstrumentationMiddleware to MetricsMiddleware.
The rename is functionally complete: file/package/constructor/wiring are all updated and no NewInstrumentationMiddleware call remains. However, two stale references to the old name survive (the TestInstrumentationMiddleware test function in metrics_middleware_test.go and the "plugin.instrumentation" base logger name in pluginsintegration.go).
⚠️ No stale references to the old Instrumentation/InstrumentationMiddleware name remain in the codebase.
Grep confirms the only remaining Instrumentation* references are the stale TestInstrumentationMiddleware test function name and the "plugin.instrumentation" base logger scope — both cosmetic.

Plugins: Chore: Renamed instrumentation middleware to metrics middleware