Skip to content
Jump to bottom

Automated Test: plugins/rename-instrumentation-middleware-to-metrics-middleware #328

Closed
wants to merge 1 commit into from
+137 −78
Closed

Automated Test: plugins/rename-instrumentation-middleware-to-metrics-middleware #328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/plugins/log/fake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package log

import "context"

var _ Logger = (*TestLogger)(nil)

type TestLogger struct {
Expand Down Expand Up @@ -41,6 +43,10 @@ func (f *TestLogger) Error(msg string, ctx ...any) {
f.ErrorLogs.Ctx = ctx
}

func (f *TestLogger) FromContext(_ context.Context) Logger {
return NewTestLogger()
}
Comment on lines +46 to +48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's look at the fake.go file around the mentioned lines
cd /tmp && find . -name "fake.go" -type f 2>/dev/null | head -20

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 68

🏁 Script executed:

# Let's find the file structure
find . -name "*.go" -path "*/log/*" -type f 2>/dev/null | head -20

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 716

🏁 Script executed:

# Search for FromContext method implementations
rg -n "func.*FromContext" --type=go -A 3

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 4484

🏁 Script executed:

# Look for LoggerMiddleware and its usage of FromContext
rg -n "LoggerMiddleware\|FromContext" --type=go -B 2 -A 2

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 68

🏁 Script executed:

# Check for test files that might assert on log fields
rg -n "InfoLogs|DebugLogs|WarnLogs|ErrorLogs" --type=go | head -30

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 1972

🏁 Script executed:

# Look for logger_middleware.go
find . -name "*logger*middleware*" -o -name "*middleware*logger*" | grep -E "\.go$"

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 381

🏁 Script executed:

# Check the Logger interface definition in plugins/log
cat pkg/plugins/log/ifaces.go

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 1329

🏁 Script executed:

# Look for tests in plugins/log directory
ls -la pkg/plugins/log/

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 444

🏁 Script executed:

# Check the fake.go file completely
cat pkg/plugins/log/fake.go

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 1775

🏁 Script executed:

# Check if logtest/fake.go and plugins/log/fake.go have same behavior
echo "=== pkg/infra/log/logtest/fake.go ===" && sed -n '66,68p' pkg/infra/log/logtest/fake.go
echo "=== pkg/plugins/log/fake.go ===" && sed -n '46,48p' pkg/plugins/log/fake.go

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 319

🏁 Script executed:

# Search for tests that might use plugins/log fake logger
find pkg/plugins -name "*test.go" -type f

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 1501

🏁 Script executed:

# Check if any tests specifically test FromContext behavior
rg -n "FromContext" pkg/plugins --type=go -B 3 -A 3

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 1390

🏁 Script executed:

# Check the logger_middleware.go file to see if it uses plugins/log
cat pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 3623

🏁 Script executed:

# Check if there are tests for this middleware
cat pkg/services/pluginsintegration/clientmiddleware/logger_middleware_test.go 2>/dev/null | head -100

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 68

🏁 Script executed:

# Search for tests in plugins directory that use TestLogger and FromContext
rg -n "TestLogger.*FromContext\|FromContext.*TestLogger" pkg/plugins --type=go -B 3 -A 3

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 68

🏁 Script executed:

# Check the infra logger implementation to see how it handles FromContext
cat pkg/infra/log/log.go | sed -n '203,220p'

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 431

🏁 Script executed:

# Check for any tests that use FromContext with fake loggers and assert on logs
rg -n "FromContext" pkg/plugins/manager/pipeline/bootstrap/steps_test.go -B 5 -A 5

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 68

🏁 Script executed:

# Check if there's a test for LoggerMiddleware
find pkg/services/pluginsintegration -name "*logger_middleware*test*" -type f

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 68

🏁 Script executed:

# Look at the test file if it exists
cat pkg/services/pluginsintegration/clientmiddleware/logger_middleware_test.go 2>/dev/null || echo "File not found"

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 100

🏁 Script executed:

# Search for any tests that instantiate TestLogger from plugins/log
rg -n "plugins.*log.*TestLogger\|log\.TestLogger" pkg --type=go -B 2 -A 5

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 68

🏁 Script executed:

# Check how plugins/log is actually used in tests vs how infra/log/logtest is used
rg -n "from.*log\.TestLogger\|plugins/log" pkg/plugins/manager/pipeline/bootstrap/steps_test.go -B 3 -A 3

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 68

🏁 Script executed:

# Let's see the actual bootstrap steps_test.go which we know uses fake loggers
cat pkg/plugins/manager/pipeline/bootstrap/steps_test.go | head -100

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 3010

🏁 Script executed:

# Check if LoggerMiddleware is actually used in tests and if tests would fail
rg -n "LoggerMiddleware" pkg/services/pluginsintegration --type=go -B 2 -A 2

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 12730

🏁 Script executed:

# Verify the complete picture: what does the real infra logger's New() method do?
cat pkg/infra/log/log.go | sed -n '213,230p'

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 513

🏁 Script executed:

# Compare the interface semantics - what should FromContext really do?
cat pkg/plugins/log/ifaces.go | sed -n '20,24p'

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 263

🏁 Script executed:

# Check if there are any integration tests that would catch this issue
find pkg/services/pluginsintegration -name "*.go" -type f | xargs grep -l "TestLogger\|LoggerMiddleware" | head -5

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 403

🏁 Script executed:

# Check if there are any tests that actually use TestLogger with LoggerMiddleware or similar FromContext calls
rg -n "TestLogger\|NewTestLogger" pkg/services/pluginsintegration --type=go

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 68

🏁 Script executed:

# Let's look at the contextual_logger_middleware to see if it also calls FromContext
cat pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 2842

🏁 Script executed:

# Final verification: check if the infra logtest fake's behavior is consistent with the real logger
cat pkg/infra/log/logtest/fake.go | sed -n '1,80p'

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 1300

FromContext returns a disconnected TestLogger, breaking potential test assertions through LoggerMiddleware.

The middleware at line 60 of logger_middleware.go calls m.logger.FromContext(ctx).Info(...). Since FromContext returns a brand-new TestLogger instance unrelated to the one tests hold a reference to, any assertions on InfoLogs.Calls, InfoLogs.Message, etc. will never capture logs written through the middleware. The interface specifies that FromContext should return "a new contextual Logger that has this logger's context"—meaning it should retain context from the original logger, not disconnect entirely. While no tests currently use TestLogger with this middleware (only production uses real loggers), this will silently break future tests.

Fix by returning the receiver:

Proposed fix 
 func (f *TestLogger) FromContext(_ context.Context) Logger {
-	return NewTestLogger()
+	return f
 }
📝 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.

Suggested change
func (f *TestLogger) FromContext(_ context.Context) Logger {
return NewTestLogger()
}
func (f *TestLogger) FromContext(_ context.Context) Logger {
return f
}
🤖 Prompt for AI Agents 
In `@pkg/plugins/log/fake.go` around lines 46 - 48, FromContext currently returns
a brand-new TestLogger, disconnecting it from the original logger and breaking
assertions; change TestLogger.FromContext to return the receiver (retain the
original TestLogger's context) instead of NewTestLogger() so LoggerMiddleware
calls like m.logger.FromContext(ctx).Info(...) operate on the same TestLogger
instance (refer to TestLogger.FromContext and LoggerMiddleware).


type Logs struct {
Calls int
Message string
Expand Down
37 changes: 21 additions & 16 deletions pkg/plugins/log/ifaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,40 @@
package log

import "context"

// Logger is the default logger
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
}

// PrettyLogger is used primarily to facilitate logging/user feedback for both
// the grafana-cli and the grafana backend when managing plugin installs
type PrettyLogger interface {
Successf(format string, args ...interface{})
Failuref(format string, args ...interface{})

Info(args ...interface{})
Infof(format string, args ...interface{})
Debug(args ...interface{})
Debugf(format string, args ...interface{})
Warn(args ...interface{})
Warnf(format string, args ...interface{})
Error(args ...interface{})
Errorf(format string, args ...interface{})
Successf(format string, args ...any)
Failuref(format string, args ...any)

Info(args ...any)
Infof(format string, args ...any)
Debug(args ...any)
Debugf(format string, args ...any)
Warn(args ...any)
Warnf(format string, args ...any)
Error(args ...any)
Errorf(format string, args ...any)
}
12 changes: 12 additions & 0 deletions pkg/plugins/log/logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package log

import (
"context"

"github.com/grafana/grafana/pkg/infra/log"
)

Expand Down Expand Up @@ -42,3 +44,13 @@ func (d *grafanaInfraLogWrapper) Warn(msg string, ctx ...any) {
func (d *grafanaInfraLogWrapper) Error(msg string, ctx ...any) {
d.l.Error(msg, ctx...)
}

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,
}
}