Skip to content
Jump to bottom

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

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

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

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()
}

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
}
Comment on lines 6 to 24
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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 -40

Repository: 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 -20

Repository: 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 -30

Repository: 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.go

Repository: 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 -50

Repository: 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 -100

Repository: 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:

  • logWrapper in pkg/plugins/backendplugin/grpcplugin/log_wrapper.go
  • logWrapper in pkg/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.


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