Plugins: Chore: Renamed instrumentation middleware to metrics middleware #8
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/plugins/log/fake.go (1)
3-4: Test logger’sFromContextcorrectly satisfies the new interfaceHaving
(*TestLogger).FromContextmirrorNewby returning a freshTestLoggeris consistent with the existing fake and enough to satisfy the extendedLoggerinterface. If you later need to assert context-derived attributes in tests, you could store thecontext.Contextor the extracted attributes on the fake, but it’s not necessary for this change.Also applies to: 46-48
pkg/plugins/log/logger.go (1)
48-55:FromContextwrapper is correct; consider minor hardeningThe implementation correctly delegates to the underlying infra logger and safely falls back to
d.New()if the type assertion fails, which avoids panics and preserves logging. You might optionally:
- Guard against a nil
ctx(e.g., returnd.New()early), and/or- Log once when the type assertion fails to help detect unexpected logger types.
Not required for correctness, but it would make this more defensive.
pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go (1)
12-57: Contextual logger middleware is well factored; consider nil-guards for requestsCentralizing the enrichment of context (endpoint, pluginId, dsName/dsUID, uname) in
ContextualLoggerMiddlewareis a nice separation of concerns and pairs well withLogger.FromContext.The only fragility is that
QueryData,CallResource,CheckHealth, andCollectMetricsall dereferencereq(viareq.PluginContext) without a nil check. If any caller ever passes a nil request, this middleware will panic even thoughLoggerMiddlewareitself has explicit nil-guards.Adding a simple guard like the following in each method would make it robust without changing behavior for valid calls:
func (m *ContextualLoggerMiddleware) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { - ctx = instrumentContext(ctx, endpointQueryData, req.PluginContext) - return m.next.QueryData(ctx, req) + if req == nil { + return m.next.QueryData(ctx, req) + } + ctx = instrumentContext(ctx, endpointQueryData, req.PluginContext) + return m.next.QueryData(ctx, req) }(and similarly for the other request types).
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go (1)
16-83: Metrics middleware rename preserves behavior; shared instance pattern is worth a quick sanity checkThe rename to
MetricsMiddlewareand the move of contextual logging out of this file keep the responsibilities clear: this middleware now focuses purely on Prometheus metrics (count, duration, size, target, and traceID exemplars). The helper functions and per-endpoint methods look consistent with the expectations inmetrics_middleware_test.go.One subtle point is the constructor pattern:
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 }) }This reuses a single
MetricsMiddlewareinstance (imw) for everynextclient created from this middleware, updating itsnextfield each time. If the surrounding decorator ever invokes this middleware multiple times with differentnextclients (e.g., multiple decorated clients in parallel), this could causeimw.nextto be overwritten.If that scenario is possible in your setup, consider returning a fresh wrapper per call while sharing the metric collectors, for example:
shared := newMetricsMiddleware(promRegisterer, pluginRegistry) return plugins.ClientMiddlewareFunc(func(next plugins.Client) plugins.Client { return &MetricsMiddleware{ pluginMetrics: shared.pluginMetrics, // reuse registered collectors pluginRegistry: shared.pluginRegistry, next: next, } })Otherwise, if there is only a single decorated client per process, the current pattern is fine but might benefit from a brief comment documenting that assumption.
Also applies to: 93-197
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/plugins/log/fake.go(2 hunks)pkg/plugins/log/ifaces.go(1 hunks)pkg/plugins/log/logger.go(2 hunks)pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go(1 hunks)pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go(7 hunks)pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go(9 hunks)pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go(1 hunks)pkg/services/pluginsintegration/pluginsintegration.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/services/pluginsintegration/pluginsintegration.go (2)
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go (1)
NewMetricsMiddleware(76-82)pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go (1)
NewContextualLoggerMiddleware(14-20)
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 (4)
pkg/plugins/log/ifaces.go (1)
3-39: Logger / PrettyLogger interface updates look soundUsing
anyinstead ofinterface{}is idiomatic and source-compatible, and addingFromContext(context.Context) Loggeraligns with the new contextual logging pipeline. Implementations in this PR (infra logger wrapper and test logger) satisfy the extended interface, so this change is consistent and safe.pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go (1)
78-83: Test constructor rename is consistent with middleware changesSwitching to
newMetricsMiddleware(promRegistry, pluginsRegistry)matches the renamed middleware type and constructor while preserving test behavior and expectations for the exposed metrics.pkg/services/pluginsintegration/pluginsintegration.go (1)
155-167: Middleware ordering aligns with tracing, metrics, and contextual logging goalsPlacing
NewMetricsMiddlewareimmediately after tracing andNewContextualLoggerMiddlewarejust beforeNewLoggerMiddlewaregives a sensible flow: tracing sets up context, metrics records timing/labels (including trace IDs), contextual middleware enriches the context with plugin attributes, and the logger then emits enriched request logs. This ordering looks correct for the intended behavior.pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go (1)
36-60: Logger now relies on contextual attributes; ensure call sites include the contextual middlewareUsing
m.logger.FromContext(ctx).Info(...)is a good fit with the new contextual logging approach and keepsLoggerMiddlewarefocused on status/duration/event fields only. The call sites are correctly updated to the simplifiedlogRequestAPI.One behavioral change is that plugin- and endpoint-specific attributes are no longer built here and are instead expected to be present in
ctx(viaContextualLoggerMiddleware). For any places whereNewLoggerMiddlewaremight be used withoutNewContextualLoggerMiddleware, logs will now lack those attributes.If you want to double-check current usage, you can run something like:
Also applies to: 62-115
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes refactor logging and middleware infrastructure in plugin integration. They introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ContextualLoggerMW as ContextualLoggerMiddleware
participant LoggerMW as LoggerMiddleware
participant MetricsMW as MetricsMiddleware
participant Backend as Backend Handler
Client->>ContextualLoggerMW: Request (ctx)
Note over ContextualLoggerMW: Enrich context with<br/>endpoint, pluginId,<br/>datasource, user
ContextualLoggerMW->>ContextualLoggerMW: log.WithContextualAttributes(ctx, attrs)
ContextualLoggerMW->>LoggerMW: Forward enriched ctx
LoggerMW->>LoggerMW: logger.FromContext(ctx).Info()
LoggerMW->>MetricsMW: Forward ctx
MetricsMW->>MetricsMW: Instrument request size & duration
MetricsMW->>Backend: Forward ctx
Backend->>MetricsMW: Response
MetricsMW->>MetricsMW: Record metrics
MetricsMW->>LoggerMW: Return response
LoggerMW->>LoggerMW: Log result via FromContext
LoggerMW->>ContextualLoggerMW: Return response
ContextualLoggerMW->>Client: Return response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go (2)
104-142: Consider broadening cancelled status and guarding exemplar type assumptionsThe core timing/metrics logic looks good, but two small refinements might make this more robust:
context.DeadlineExceededis currently reported asstatusError. If you conceptually treat timeouts as cancellations, you may want to map it tostatusCancelledalongsidecontext.Canceled.- The exemplar paths rely on
WithLabelValues(...)returning types that implementprometheus.ExemplarObserver/ExemplarAdder. If the metric types are ever changed, these hard type assertions will panic; a defensiveokcheck (or keeping the fields as concrete histogram/counter vecs and using their typed methods) would make this future‑proof.
187-197: Streams are passed through without metrics; consider if they should be instrumented
SubscribeStream,PublishStream, andRunStreamare currently transparent pass‑throughs and don’t emit any metrics. If streaming usage becomes important operationally, it may be worth adding basic duration/erroneous-call instrumentation similar to the request/response paths; otherwise, this simple delegation is fine.pkg/plugins/log/fake.go (1)
3-4: TestLogger.FromContext satisfies the interface but discards existing log stateAdding
FromContextkeepsTestLoggerconformant toLogger, and the implementation is simple. Note that returningNewTestLogger()means any logs emitted via the context‑derived logger won’t update the originalTestLogger’sLogscounters.If you ever need tests to assert on logs coming from both the base and context‑derived logger, consider returning
finstead of a new instance:-func (f *TestLogger) FromContext(_ context.Context) Logger { - return NewTestLogger() -} +func (f *TestLogger) FromContext(_ context.Context) Logger { + return f +}Also applies to: 46-48
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go (1)
21-22: Test wiring to newMetricsMiddleware is correct; consider renaming the testThe test now correctly uses
newMetricsMiddleware(promRegistry, pluginsRegistry)and still validates the expected metrics, so behavior is preserved.For clarity and future grepability, you might want to rename
TestInstrumentationMiddlewaretoTestMetricsMiddleware(and similar identifiers, if any) to align with the new middleware name, but this is purely cosmetic.Also applies to: 72-85
pkg/plugins/log/logger.go (1)
3-7: Defensive nil check is good practice, but current code is not brokenThe verification shows
ConcreteLogger.FromContextalways returns non-nil: it either returnscl.New(args...)orclitself (lines 203-217). The type assertion will not fail silently with a nil pointer in practice.However, your suggestion for an explicit nil check remains valid defensive programming. If the underlying logger's
FromContextever changes behavior, the nil guard prevents latent panics in methods likeInfo(),Warn(), andError()that unconditionally dereferenced.l.The refactor is optional but recommended:
func (d *grafanaInfraLogWrapper) FromContext(ctx context.Context) Logger { - concreteInfraLogger, ok := d.l.FromContext(ctx).(*log.ConcreteLogger) - if !ok { - return d.New() - } - return &grafanaInfraLogWrapper{ - l: concreteInfraLogger, - } + if l, ok := d.l.FromContext(ctx).(*log.ConcreteLogger); ok && l != nil { + return &grafanaInfraLogWrapper{l: l} + } + return d }Adjust fallback to
d(reuse current wrapper) ord.New()(new child logger) based on intent.Also applies to: 48-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/plugins/log/fake.go(2 hunks)pkg/plugins/log/ifaces.go(1 hunks)pkg/plugins/log/logger.go(2 hunks)pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go(1 hunks)pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go(7 hunks)pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go(9 hunks)pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go(1 hunks)pkg/services/pluginsintegration/pluginsintegration.go(1 hunks)
🔇 Additional comments (6)
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go (2)
24-30: MetricsMiddleware rename and constructor wiring look consistentThe rename to
MetricsMiddlewareand thenewMetricsMiddleware/NewMetricsMiddlewareconstructor path keep the metrics’ names, labels, and registration behavior intact, so existing dashboards/alerts should continue to work as before. Reusing a single middleware instance via the closure preserves the previous aggregated metrics behavior.Also applies to: 32-73, 75-82
144-185: Per-endpoint metrics instrumentation remains coherent
QueryData,CallResource,CheckHealth, andCollectMetricsall use the sharedinstrumentPluginRequesthelper and, where applicable,instrumentPluginRequestSize, which keeps the metrics shape and labeling consistent across endpoints while centralizing error/status handling. This is a clean factoring and matches the intended metrics behavior.pkg/services/pluginsintegration/pluginsintegration.go (1)
155-167: Middleware chain update (metrics + contextual logger) is sensiblePlacing
NewMetricsMiddleware(promRegisterer, registry)immediately after tracing, followed byNewContextualLoggerMiddleware()and thenNewLoggerMiddleware, gives a clear separation of concerns: tracing and metrics first, then logger context enrichment, then actual logging and the rest of the client middlewares. This ordering is coherent and doesn’t obviously change behavior beyond the intended rename/refactor.pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go (1)
36-60: Context-based logger usage looks consistent and safe
logRequestnow relies onlogger.FromContext(ctx)and the call sites correctly pass the samectxthrough the closure, while preserving the priornil-request short‑circuit behavior. No new correctness or panic risks introduced here.Also applies to: 62-74, 76-87, 89-101, 103-115
pkg/plugins/log/ifaces.go (1)
3-24: Logger / PrettyLogger interface updates align with new contextual loggingUsing
anyfor variadics is idiomatic in modern Go and consistent across both interfaces, and the newFromContext(context.Context) Loggermethod matches howLoggerMiddlewarenow consumes loggers from context. Just be aware this is a required method for all existingLoggerimplementations.Also applies to: 26-40
pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go (1)
26-37: > Likely an incorrect or invalid review comment.
| func (m *ContextualLoggerMiddleware) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { | ||
| ctx = instrumentContext(ctx, endpointQueryData, req.PluginContext) | ||
| return m.next.QueryData(ctx, req) | ||
| } | ||
|
|
||
| func (m *ContextualLoggerMiddleware) CallResource(ctx context.Context, req *backend.CallResourceRequest, sender backend.CallResourceResponseSender) error { | ||
| ctx = instrumentContext(ctx, endpointCallResource, req.PluginContext) | ||
| return m.next.CallResource(ctx, req, sender) | ||
| } | ||
|
|
||
| func (m *ContextualLoggerMiddleware) CheckHealth(ctx context.Context, req *backend.CheckHealthRequest) (*backend.CheckHealthResult, error) { | ||
| ctx = instrumentContext(ctx, endpointCheckHealth, req.PluginContext) | ||
| return m.next.CheckHealth(ctx, req) | ||
| } | ||
|
|
||
| func (m *ContextualLoggerMiddleware) CollectMetrics(ctx context.Context, req *backend.CollectMetricsRequest) (*backend.CollectMetricsResult, error) { | ||
| ctx = instrumentContext(ctx, endpointCollectMetrics, req.PluginContext) | ||
| return m.next.CollectMetrics(ctx, req) | ||
| } |
There was a problem hiding this comment.
Guard against nil requests to avoid panics in ContextualLoggerMiddleware
Each of these methods dereferences req (via req.PluginContext) without a nil check, unlike LoggerMiddleware, which explicitly handles req == nil. If any existing caller can pass a nil request, this middleware will now panic.
Recommend short‑circuiting to next when req is nil, mirroring LoggerMiddleware:
func (m *ContextualLoggerMiddleware) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
- ctx = instrumentContext(ctx, endpointQueryData, req.PluginContext)
- return m.next.QueryData(ctx, req)
+ if req == nil {
+ return m.next.QueryData(ctx, req)
+ }
+ ctx = instrumentContext(ctx, endpointQueryData, req.PluginContext)
+ return m.next.QueryData(ctx, req)
}
func (m *ContextualLoggerMiddleware) CallResource(ctx context.Context, req *backend.CallResourceRequest, sender backend.CallResourceResponseSender) error {
- ctx = instrumentContext(ctx, endpointCallResource, req.PluginContext)
- return m.next.CallResource(ctx, req, sender)
+ if req == nil {
+ return m.next.CallResource(ctx, req, sender)
+ }
+ ctx = instrumentContext(ctx, endpointCallResource, req.PluginContext)
+ return m.next.CallResource(ctx, req, sender)
}
func (m *ContextualLoggerMiddleware) CheckHealth(ctx context.Context, req *backend.CheckHealthRequest) (*backend.CheckHealthResult, error) {
- ctx = instrumentContext(ctx, endpointCheckHealth, req.PluginContext)
- return m.next.CheckHealth(ctx, req)
+ if req == nil {
+ return m.next.CheckHealth(ctx, req)
+ }
+ ctx = instrumentContext(ctx, endpointCheckHealth, req.PluginContext)
+ return m.next.CheckHealth(ctx, req)
}
func (m *ContextualLoggerMiddleware) CollectMetrics(ctx context.Context, req *backend.CollectMetricsRequest) (*backend.CollectMetricsResult, error) {
- ctx = instrumentContext(ctx, endpointCollectMetrics, req.PluginContext)
- return m.next.CollectMetrics(ctx, req)
+ if req == nil {
+ return m.next.CollectMetrics(ctx, req)
+ }
+ ctx = instrumentContext(ctx, endpointCollectMetrics, req.PluginContext)
+ return m.next.CollectMetrics(ctx, req)
}This keeps behavior consistent with the existing logger middleware and avoids introducing a crash surface.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *ContextualLoggerMiddleware) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { | |
| ctx = instrumentContext(ctx, endpointQueryData, req.PluginContext) | |
| return m.next.QueryData(ctx, req) | |
| } | |
| func (m *ContextualLoggerMiddleware) CallResource(ctx context.Context, req *backend.CallResourceRequest, sender backend.CallResourceResponseSender) error { | |
| ctx = instrumentContext(ctx, endpointCallResource, req.PluginContext) | |
| return m.next.CallResource(ctx, req, sender) | |
| } | |
| func (m *ContextualLoggerMiddleware) CheckHealth(ctx context.Context, req *backend.CheckHealthRequest) (*backend.CheckHealthResult, error) { | |
| ctx = instrumentContext(ctx, endpointCheckHealth, req.PluginContext) | |
| return m.next.CheckHealth(ctx, req) | |
| } | |
| func (m *ContextualLoggerMiddleware) CollectMetrics(ctx context.Context, req *backend.CollectMetricsRequest) (*backend.CollectMetricsResult, error) { | |
| ctx = instrumentContext(ctx, endpointCollectMetrics, req.PluginContext) | |
| return m.next.CollectMetrics(ctx, req) | |
| } | |
| func (m *ContextualLoggerMiddleware) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { | |
| if req == nil { | |
| return m.next.QueryData(ctx, req) | |
| } | |
| ctx = instrumentContext(ctx, endpointQueryData, req.PluginContext) | |
| return m.next.QueryData(ctx, req) | |
| } | |
| func (m *ContextualLoggerMiddleware) CallResource(ctx context.Context, req *backend.CallResourceRequest, sender backend.CallResourceResponseSender) error { | |
| if req == nil { | |
| return m.next.CallResource(ctx, req, sender) | |
| } | |
| ctx = instrumentContext(ctx, endpointCallResource, req.PluginContext) | |
| return m.next.CallResource(ctx, req, sender) | |
| } | |
| func (m *ContextualLoggerMiddleware) CheckHealth(ctx context.Context, req *backend.CheckHealthRequest) (*backend.CheckHealthResult, error) { | |
| if req == nil { | |
| return m.next.CheckHealth(ctx, req) | |
| } | |
| ctx = instrumentContext(ctx, endpointCheckHealth, req.PluginContext) | |
| return m.next.CheckHealth(ctx, req) | |
| } | |
| func (m *ContextualLoggerMiddleware) CollectMetrics(ctx context.Context, req *backend.CollectMetricsRequest) (*backend.CollectMetricsResult, error) { | |
| if req == nil { | |
| return m.next.CollectMetrics(ctx, req) | |
| } | |
| ctx = instrumentContext(ctx, endpointCollectMetrics, req.PluginContext) | |
| return m.next.CollectMetrics(ctx, req) | |
| } |
🤖 Prompt for AI Agents
In
pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go
around lines 39 to 57, the methods QueryData, CallResource, CheckHealth, and
CollectMetrics dereference req.PluginContext without checking if req is nil; add
a nil-check at the start of each method and short-circuit by calling and
returning m.next.<method>(ctx, req[, sender]) when req == nil (matching the
existing LoggerMiddleware behavior) to avoid panics and keep behavior
consistent.
Test 3
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/grafana-coderabbit#3