Automated Test: plugins/rename-instrumentation-middleware-to-metrics-middleware #346
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
📝 WalkthroughWalkthroughThe changes add a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Plugin Client
participant CLM as ContextualLoggerMiddleware
participant LM as LoggerMiddleware
participant PM as MetricsMiddleware
participant Handler as Underlying Handler
Client->>CLM: QueryData(ctx, req)
CLM->>CLM: instrumentContext(ctx)
CLM->>CLM: Enrich context with<br/>endpoint, pluginId,<br/>dsName, dsUID, uname
CLM->>LM: QueryData(enriched_ctx, req)
LM->>LM: logRequest(ctx, fn)
LM->>LM: logger.FromContext(ctx)
LM->>PM: QueryData(ctx, req)
PM->>PM: Record metrics
PM->>Handler: QueryData(ctx, req)
Handler-->>PM: response
PM-->>LM: response
LM-->>CLM: response
CLM-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/plugins/log/ifaces.go`:
- Around line 6-24: The Logger interface now requires
FromContext(context.Context) Logger; update both logWrapper types named
logWrapper in pkg/plugins/backendplugin/grpcplugin/log_wrapper.go and
pkg/plugins/backendplugin/coreplugin/registry.go to implement FromContext by
delegating to their embedded/field Logger (call the underlying logger's
FromContext and return the resulting Logger), ensuring the method signature
matches Logger.FromContext and returns the contextual wrapper if needed to
preserve wrapper behavior.
🧹 Nitpick comments (5)
pkg/plugins/log/fake.go (1)
46-48:FromContextreturns a disconnected logger — log assertions may silently miss entries.
FromContextreturns a brand-newTestLoggerwith its ownLogsfields, just likeNewdoes. Any code path that callslogger.FromContext(ctx).Info(...)(e.g.,LoggerMiddleware.logRequest) will write to a logger that no test holds a reference to, so assertions on the originalTestLogger'sInfoLogs,ErrorLogs, etc. will never see those entries.If test coverage for
LoggerMiddlewarelog output matters, consider returning the receiver (f) instead, or at least documenting this behavior.Possible fix (return receiver)
func (f *TestLogger) FromContext(_ context.Context) Logger { - return NewTestLogger() + return f }pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go (1)
21-21: Test function name is stale after the rename.The type was renamed from
InstrumentationMiddlewaretoMetricsMiddleware, but the test function is stillTestInstrumentationMiddleware. Consider renaming it toTestMetricsMiddlewarefor consistency.pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go (1)
75-82: Stale variable nameimw(instrumentation middleware).
imwis a leftover abbreviation fromInstrumentationMiddleware. Consider renaming tommwormwfor consistency with theMetricsMiddlewaretype.Suggested rename
func NewMetricsMiddleware(promRegisterer prometheus.Registerer, pluginRegistry registry.Service) plugins.ClientMiddleware { - imw := newMetricsMiddleware(promRegisterer, pluginRegistry) + mw := newMetricsMiddleware(promRegisterer, pluginRegistry) return plugins.ClientMiddlewareFunc(func(next plugins.Client) plugins.Client { - imw.next = next - return imw + mw.next = next + return mw }) }pkg/plugins/log/ifaces.go (1)
21-23: Doc comment forFromContextis identical toNewand doesn't convey the distinction.Line 22 says "returns a new contextual Logger that has this logger's context plus the given context" — the same wording as
Newon line 7.FromContextretrieves logger attributes stored in acontext.Context(e.g., by middleware), whereasNewaccepts explicit key/value pairs. Consider making the doc more specific.Suggested doc
- // FromContext returns a new contextual Logger that has this logger's context plus the given context. + // FromContext returns a Logger enriched with attributes previously stored in the given context. FromContext(ctx context.Context) Loggerpkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go (1)
59-69: Consider instrumenting stream methods for consistency.
SubscribeStream,PublishStream, andRunStreamall have access toreq.PluginContextbut skipinstrumentContext. This means downstream middleware/handlers for streaming won't have the contextual logging attributes. If this omission is intentional (e.g., long-lived streams where per-request context is less useful), a brief comment explaining the rationale would help future readers.
| type Logger interface { | ||
| // New returns a new contextual Logger that has this logger's context plus the given context. | ||
| New(ctx ...interface{}) Logger | ||
| New(ctx ...any) Logger | ||
|
|
||
| // Debug logs a message with debug level and key/value pairs, if any. | ||
| Debug(msg string, ctx ...interface{}) | ||
| Debug(msg string, ctx ...any) | ||
|
|
||
| // Info logs a message with info level and key/value pairs, if any. | ||
| Info(msg string, ctx ...interface{}) | ||
| Info(msg string, ctx ...any) | ||
|
|
||
| // Warn logs a message with warning level and key/value pairs, if any. | ||
| Warn(msg string, ctx ...interface{}) | ||
| Warn(msg string, ctx ...any) | ||
|
|
||
| // Error logs a message with error level and key/value pairs, if any. | ||
| Error(msg string, ctx ...interface{}) | ||
| Error(msg string, ctx ...any) | ||
|
|
||
| // FromContext returns a new contextual Logger that has this logger's context plus the given context. | ||
| FromContext(ctx context.Context) Logger | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if Logger interface is used/implemented outside the plugins/log package
rg -n --type=go 'log\.Logger' -g '!pkg/plugins/log/*' -g '!vendor/*' | head -40Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 4598
🏁 Script executed:
# Also search for direct implementations of the Logger interface
# Look for methods like Debug, Info, Warn, Error to find implementations
rg -n --type=go 'func.*Debug.*msg string' -g '!vendor/*' | head -20Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 742
🏁 Script executed:
# Search for any type implementing Logger interface patterns
rg -n --type=go 'type.*struct' -A 5 | grep -A 5 'Debug\|Info\|Warn\|Error' | head -30Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1569
🏁 Script executed:
# Check the file to understand the full context
cat -n pkg/plugins/log/ifaces.goRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1609
🏁 Script executed:
# Check each implementation for FromContext method
echo "=== Checking logWrapper in grpcplugin ===" && \
rg -A 30 'type logWrapper' pkg/plugins/backendplugin/grpcplugin/log_wrapper.go | head -50
echo -e "\n=== Checking logWrapper in coreplugin ===" && \
rg -A 30 'type logWrapper' pkg/plugins/backendplugin/coreplugin/registry.go | head -50
echo -e "\n=== Checking grafanaInfraLogWrapper ===" && \
rg -A 30 'type grafanaInfraLogWrapper' pkg/plugins/log/logger.go | head -50
echo -e "\n=== Checking TestLogger ===" && \
rg -A 30 'type TestLogger' pkg/plugins/log/fake.go | head -50
echo -e "\n=== Checking Fake logger ===" && \
rg -A 30 'type Fake' pkg/infra/log/logtest/fake.go | head -50
echo -e "\n=== Checking ConcreteLogger ===" && \
rg -A 30 'type ConcreteLogger' pkg/infra/log/log.go | head -50Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 3884
🏁 Script executed:
# Check if FromContext method exists in any of these implementation files
echo "=== Searching for FromContext implementations ===" && \
rg -n 'FromContext' pkg/plugins/ pkg/infra/log/Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1691
🏁 Script executed:
# Check if the implementations in grpcplugin and coreplugin have FromContext
echo "=== Checking grpcplugin/log_wrapper.go for FromContext ===" && \
rg -n 'FromContext' pkg/plugins/backendplugin/grpcplugin/log_wrapper.go
echo -e "\n=== Checking coreplugin/registry.go for FromContext ===" && \
rg -n 'FromContext' pkg/plugins/backendplugin/coreplugin/registry.go
# Get the full implementation of both logWrapper types to see all methods
echo -e "\n=== Full logWrapper in grpcplugin ===" && \
cat -n pkg/plugins/backendplugin/grpcplugin/log_wrapper.go
echo -e "\n=== Full logWrapper in coreplugin ===" && \
rg -B 5 -A 100 'type logWrapper struct' pkg/plugins/backendplugin/coreplugin/registry.go | head -100Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 6227
Update internal Logger interface implementations to include the FromContext method.
Adding FromContext to the Logger interface is a breaking change. Two internal adapter types are missing the implementation and will fail to compile:
logWrapperinpkg/plugins/backendplugin/grpcplugin/log_wrapper.gologWrapperinpkg/plugins/backendplugin/coreplugin/registry.go
Both should delegate to their underlying Logger field's FromContext method to satisfy the interface.
🤖 Prompt for AI Agents
In `@pkg/plugins/log/ifaces.go` around lines 6 - 24, The Logger interface now
requires FromContext(context.Context) Logger; update both logWrapper types named
logWrapper in pkg/plugins/backendplugin/grpcplugin/log_wrapper.go and
pkg/plugins/backendplugin/coreplugin/registry.go to implement FromContext by
delegating to their embedded/field Logger (call the underlying logger's
FromContext and return the resulting Logger), ensuring the method signature
matches Logger.FromContext and returns the contextual wrapper if needed to
preserve wrapper behavior.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Improvements