Plugins: Chore: Renamed instrumentation middleware to metrics middleware#4
Conversation
…are (#76186) * Plugins: Chore: Renamed instrumentation middleware to metrics middleware * Removed repeated logger attributes in middleware and contextual logger * renamed loggerParams to logParams * PR review suggestion * Add contextual logger middleware * Removed unused params from logRequest * Removed unwanted changes * Safer FromContext method * Removed traceID from logParams
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis change standardizes the logging API by replacing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go (1)
59-69: Streaming methods skip contextual logging instrumentation.Unlike the non-streaming methods (
QueryData,CallResource, etc.), these streaming methods (SubscribeStream,PublishStream,RunStream) do not callinstrumentContext. If this is intentional due to different lifecycle semantics of streaming operations, consider adding a brief comment explaining why. Otherwise, consider adding instrumentation for consistency.pkg/plugins/log/fake.go (1)
46-48: Test fake discards context - consider preserving for assertions.The
FromContextimplementation returns a freshTestLogger, discarding any contextual attributes stored in the provided context. While acceptable for a simple fake, this means tests using this logger won't be able to verify that contextual attributes were properly propagated.Consider storing the context or returning the same logger instance if tests need to assert on contextual logging behavior:
♻️ Optional: preserve context for test assertions
-func (f *TestLogger) FromContext(_ context.Context) Logger { - return NewTestLogger() +func (f *TestLogger) FromContext(ctx context.Context) Logger { + // Return same instance to allow tests to capture logs with context + return f }pkg/plugins/log/logger.go (1)
48-56: Silent fallback may obscure configuration issues.When the type assertion fails (line 49-51), the method silently returns a new logger via
d.New(), potentially losing contextual attributes. While this "safe" fallback prevents panics, consider logging a warning when this fallback occurs to aid debugging if contextual logging unexpectedly stops working.♻️ Optional: add debug logging on fallback
func (d *grafanaInfraLogWrapper) FromContext(ctx context.Context) Logger { concreteInfraLogger, ok := d.l.FromContext(ctx).(*log.ConcreteLogger) if !ok { + d.l.Debug("FromContext: unexpected logger type, falling back to new logger") return d.New() } return &grafanaInfraLogWrapper{ l: concreteInfraLogger, } }pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go (1)
21-21: Consider renaming test function to match middleware rename.The test function is still named
TestInstrumentationMiddlewarewhile the middleware has been renamed toMetricsMiddleware. For consistency and discoverability, consider renaming toTestMetricsMiddleware.♻️ Suggested rename
-func TestInstrumentationMiddleware(t *testing.T) { +func TestMetricsMiddleware(t *testing.T) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/plugins/log/fake.gopkg/plugins/log/ifaces.gopkg/plugins/log/logger.gopkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.gopkg/services/pluginsintegration/clientmiddleware/logger_middleware.gopkg/services/pluginsintegration/clientmiddleware/metrics_middleware.gopkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.gopkg/services/pluginsintegration/pluginsintegration.go
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go (1)
pkg/services/contexthandler/contexthandler.go (1)
FromContext(46-51)
pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go (2)
pkg/plugins/ifaces.go (2)
ClientMiddleware(142-145)ClientMiddlewareFunc(150-150)pkg/infra/log/log.go (1)
WithContextualAttributes(285-291)
pkg/plugins/log/ifaces.go (2)
pkg/plugins/log/logger.go (1)
New(9-13)pkg/services/contexthandler/contexthandler.go (1)
FromContext(46-51)
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go (3)
pkg/plugins/ifaces.go (2)
ClientMiddleware(142-145)ClientMiddlewareFunc(150-150)pkg/plugins/errors.go (1)
ErrPluginNotRegistered(9-9)pkg/tsdb/prometheus/querydata/request.go (1)
QueryData(37-47)
🔇 Additional comments (11)
pkg/services/pluginsintegration/pluginsintegration.go (1)
157-167: LGTM! Middleware chain order is logical.The middleware ordering is sensible:
TracingMiddleware- establishes trace context firstMetricsMiddleware- records metrics including durationContextualLoggerMiddleware- enriches context with logging attributesLoggerMiddleware- consumes enriched context for loggingThis ensures logging attributes are available to downstream middlewares and the logger.
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go (1)
78-84: LGTM!The constructor call correctly updated to use
newMetricsMiddleware, consistent with the middleware rename.pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go (2)
12-42: LGTM! Middleware implementation follows established patterns.The constructor and core instrumentation logic are well-structured. The use of
log.WithContextualAttributescorrectly enriches the context with logging metadata, and the endpoint constants are properly defined.
26-37: No actionable concerns. The logging of user login information viapCtx.User.Loginis consistent with existing patterns throughout the codebase (e.g.,pkg/api/featuremgmt.go,pkg/api/pluginproxy/), indicating this is an accepted logging practice.Likely an incorrect or invalid review comment.
pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go (3)
62-74: LGTM!The
QueryDatamethod correctly delegates tologRequestwith the context and inner operation. The nil-check early return is preserved.
76-127: LGTM!All remaining methods follow the established pattern consistently. The streaming methods (
SubscribeStream,PublishStream,RunStream) intentionally bypass logging, which aligns with the design to only instrument request-response style operations.
36-60: Middleware ordering dependency confirmed.The refactored
logRequestcorrectly relies onFromContext(ctx)to obtain a logger with contextual metadata (endpoint, pluginId, dsName, dsUID, uname) injected byContextualLoggerMiddleware. This achieves clean separation of concerns between context enrichment and logging.The middleware chain in
pluginsintegration.gocorrectly placesContextualLoggerMiddleware(line 160) beforeLoggerMiddleware(line 161), ensuring context is enriched before logging occurs.pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go (2)
84-142: LGTM!The helper methods are well-structured:
pluginTargetcorrectly handles the case where the plugin is not registeredinstrumentPluginRequestSizeproperly tracks request sizesinstrumentPluginRequesthandles timing, status tracking, and Prometheus exemplars for tracing correlation
144-197: LGTM!The interface methods are consistently implemented. The rename from
InstrumentationMiddlewaretoMetricsMiddlewarebetter reflects the purpose (Prometheus metrics collection). Streaming methods appropriately pass through without instrumentation.pkg/plugins/log/ifaces.go (2)
26-40: LGTM!Consistent modernization of
PrettyLoggerinterface to useanyinstead ofinterface{}.
3-24: Good modernization with context-aware logging properly implemented across all components.The changes to the
Loggerinterface are well-designed:
- Using
anyinstead ofinterface{}aligns with Go 1.18+ idiomatic style- The new
FromContext(ctx context.Context) Loggermethod enables context-aware logging, allowing middleware to inject contextual metadataAll Logger implementations have been properly updated with the
FromContextmethod: bothgrafanaInfraLogWrapperandTestLoggerinclude the required implementation.
| // NewMetricsMiddleware returns a new MetricsMiddleware. | ||
| func NewMetricsMiddleware(promRegisterer prometheus.Registerer, pluginRegistry registry.Service) plugins.ClientMiddleware { | ||
| imw := newMetricsMiddleware(promRegisterer, pluginRegistry) | ||
| return plugins.ClientMiddlewareFunc(func(next plugins.Client) plugins.Client { | ||
| imw.next = next | ||
| return imw | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'NewMetricsMiddleware' --type go -A 3 -B 1Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 1457
🏁 Script executed:
# Also check the metrics registration in newMetricsMiddleware
rg -n 'newMetricsMiddleware' --type go -A 20Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 7992
🏁 Script executed:
# Check if there are any tests for this middleware
fd -e test.go | xargs rg -l 'MetricsMiddleware|metrics_middleware' --type goRepository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 718
🏁 Script executed:
# Find where the middleware list is used after creation
rg -n 'clientmiddleware\.New' --type go -B 2 -A 10Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 3072
🏁 Script executed:
# Check how middlewares are applied to clients
rg -n 'ClientMiddleware' --type go -A 5 | head -100Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 10945
🏁 Script executed:
# Check how the middleware list is used - find where it's passed/applied
rg -n 'CreateMiddlewares\|middlewares' --type go -B 2 -A 5 | grep -A 10 'CreateMiddlewares'Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 65
🏁 Script executed:
# Look for plugin client creation/wrapping logic
rg -n 'ClientMiddlewareFunc|plugins\.Client' --type go -B 2 -A 8 | head -120Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 8090
🏁 Script executed:
# Check the ClientMiddleware interface definition
rg -n 'type ClientMiddleware' --type go -A 3Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 630
🏁 Script executed:
# Find where the middlewares are used after CreateMiddlewares returns
rg -n 'CreateMiddlewares' --type go -B 2 -A 15Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 4121
🏁 Script executed:
# Look for how many plugin clients are created and if they share middlewares
rg -n 'NewDecorator\|clientFromMiddlewares' --type go -B 3 -A 8Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 65
🏁 Script executed:
# Check how NewDecorator is used - is it called once or multiple times?
rg -n 'NewDecorator' --type go -B 2 -A 5Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 3823
🏁 Script executed:
# Look at the Decorator implementation to understand the flow better
cat -n pkg/plugins/manager/client/decorator.go | head -50Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 1680
🏁 Script executed:
# Look at how clientFromMiddlewares is called - is it called per request?
cat -n pkg/plugins/manager/client/decorator.go | sed -n '29,75p'Repository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 1834
🏁 Script executed:
# Check how many methods call clientFromMiddlewares
rg -n 'clientFromMiddlewares' pkg/plugins/manager/client/decorator.goRepository: AI-Code-Review-Evals/coderabbit-grafana
Length of output: 625
Race condition when middleware is used with concurrent requests.
The NewMetricsMiddleware factory creates a single *MetricsMiddleware instance and returns a closure that sets imw.next each time it's invoked. The closure is called once per request via clientFromMiddlewares() (called in every Decorator method: QueryData, CallResource, CollectMetrics, CheckHealth, SubscribeStream, PublishStream, CallRoute). With concurrent requests, multiple goroutines will simultaneously write to imw.next, causing a data race and potential request routing errors.
Compare to other middlewares like NewTracingMiddleware and NewCachingMiddleware, which correctly create a new instance per closure invocation. The fix requires creating a new middleware instance per request while registering Prometheus metrics once at initialization.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
1 participant
Test 3nn
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/grafana-coderabbit#3](https://github.com/ai-code-review-evaluation/grafana-coderabbit/pull/3)*