Add logging stack with slog adapter #1574
Conversation
This allows us to have a deeper observability stack in the MCP server, particularly for surfacing logging in the remote MCP server.
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive logging infrastructure for the MCP server to enable deeper observability, particularly for the remote MCP server deployment. The implementation adds a new pkg/observability package with an slog-based logging abstraction layer.
Key changes:
- Introduces a new observability package with Logger interface and two implementations (SlogLogger and NoopLogger)
- Modifies exported API signatures to thread observability through the system (DefaultToolsetGroup, IssueRead)
- Integrates logging into the server initialization and tool handlers
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/observability/observability.go | Introduces Exporters struct to hold observability components (currently just Logger) |
| pkg/observability/log/log.go | Defines Logger interface with standard logging methods and level support |
| pkg/observability/log/level.go | Defines custom log Level type with constants for debug through fatal |
| pkg/observability/log/slog_adapter.go | Implements Logger interface as adapter for Go's standard slog.Logger |
| pkg/observability/log/noop_adapter.go | Implements no-op Logger for testing and scenarios where logging is disabled |
| pkg/github/tools.go | Updates DefaultToolsetGroup to accept observability.Exporters parameter |
| pkg/github/issues.go | Updates IssueRead to accept observability.Exporters and adds logging statement |
| pkg/github/issues_test.go | Updates test setup to create and pass NoopLogger through observability.Exporters |
| internal/ghmcp/server.go | Integrates slog adapter into server initialization, wrapping existing slog.Logger |
| cmd/github-mcp-server/generate_docs.go | Updates documentation generator to pass NoopLogger for observability parameter |
| func (l *SlogLogger) Fatal(msg string, fields ...slog.Attr) { | ||
| l.Log(context.Background(), FatalLevel, msg, fields...) | ||
| panic("fatal log called") | ||
| } |
There was a problem hiding this comment.
The Fatal method panics unconditionally after logging, which means the log message may not be flushed to its destination before the panic occurs. Since slog buffers writes, the fatal log entry could be lost. Consider calling Sync() on the underlying logger before panicking, or use os.Exit(1) instead of panic to allow deferred functions and cleanup to run.
| func (l *NoopLogger) Fatal(_ string, _ ...slog.Attr) { | ||
| // No-op |
There was a problem hiding this comment.
The NoopLogger's Fatal method should maintain consistent behavior with the SlogLogger implementation. Since SlogLogger panics after logging a fatal message, NoopLogger should also panic to ensure consistent Fatal semantics across implementations. Otherwise, code that relies on Fatal terminating execution could continue running unexpectedly when using NoopLogger.
| func (l *NoopLogger) Fatal(_ string, _ ...slog.Attr) { | |
| // No-op | |
| func (l *NoopLogger) Fatal(msg string, _ ...slog.Attr) { | |
| panic("NoopLogger.Fatal called: " + msg) |
| func NewSlogLogger(logger *slog.Logger, level Level) *SlogLogger { | ||
| return &SlogLogger{ | ||
| logger: logger, | ||
| level: level, | ||
| } | ||
| } | ||
|
|
||
| func (l *SlogLogger) Level() Level { | ||
| return l.level | ||
| } | ||
|
|
||
| func (l *SlogLogger) Log(ctx context.Context, level Level, msg string, fields ...slog.Attr) { | ||
| slogLevel := convertLevel(level) | ||
| l.logger.LogAttrs(ctx, slogLevel, msg, fields...) | ||
| } | ||
|
|
||
| func (l *SlogLogger) Debug(msg string, fields ...slog.Attr) { | ||
| l.Log(context.Background(), DebugLevel, msg, fields...) | ||
| } | ||
|
|
||
| func (l *SlogLogger) Info(msg string, fields ...slog.Attr) { | ||
| l.Log(context.Background(), InfoLevel, msg, fields...) | ||
| } | ||
|
|
||
| func (l *SlogLogger) Warn(msg string, fields ...slog.Attr) { | ||
| l.Log(context.Background(), WarnLevel, msg, fields...) | ||
| } | ||
|
|
||
| func (l *SlogLogger) Error(msg string, fields ...slog.Attr) { | ||
| l.Log(context.Background(), ErrorLevel, msg, fields...) | ||
| } | ||
|
|
||
| func (l *SlogLogger) Fatal(msg string, fields ...slog.Attr) { | ||
| l.Log(context.Background(), FatalLevel, msg, fields...) | ||
| panic("fatal log called") | ||
| } | ||
|
|
||
| func (l *SlogLogger) WithFields(fields ...slog.Attr) Logger { | ||
| fieldKvPairs := make([]any, 0, len(fields)*2) | ||
| for _, attr := range fields { | ||
| k, v := attr.Key, attr.Value | ||
| fieldKvPairs = append(fieldKvPairs, k, v.Any()) | ||
| } | ||
| return &SlogLogger{ | ||
| logger: l.logger.With(fieldKvPairs...), | ||
| level: l.level, | ||
| } | ||
| } | ||
|
|
||
| func (l *SlogLogger) WithError(err error) Logger { | ||
| return &SlogLogger{ | ||
| logger: l.logger.With("error", err.Error()), | ||
| level: l.level, | ||
| } | ||
| } | ||
|
|
||
| func (l *SlogLogger) Named(name string) Logger { | ||
| return &SlogLogger{ | ||
| logger: l.logger.With("logger", name), | ||
| level: l.level, | ||
| } | ||
| } | ||
|
|
||
| func (l *SlogLogger) WithLevel(level Level) Logger { | ||
| return &SlogLogger{ | ||
| logger: l.logger, | ||
| level: level, | ||
| } | ||
| } | ||
|
|
||
| func (l *SlogLogger) Sync() error { | ||
| // Slog does not require syncing | ||
| return nil | ||
| } | ||
|
|
||
| func convertLevel(level Level) slog.Level { | ||
| switch level { | ||
| case DebugLevel: | ||
| return slog.LevelDebug | ||
| case InfoLevel: | ||
| return slog.LevelInfo | ||
| case WarnLevel: | ||
| return slog.LevelWarn | ||
| case ErrorLevel: | ||
| return slog.LevelError | ||
| case FatalLevel: | ||
| return slog.LevelError | ||
| default: | ||
| return slog.LevelInfo | ||
| } | ||
| } |
There was a problem hiding this comment.
The SlogLogger adapter lacks test coverage for its core functionality including level conversion, field marshaling, error handling, and logger chaining. Similar packages in this repository (like pkg/log and pkg/sanitize) have comprehensive test files. Consider adding tests to verify: level conversion accuracy, WithFields properly converts slog.Attr to key-value pairs, WithError extracts error strings correctly, and logger chaining with Named/WithLevel preserves the underlying logger.
| func NewNoopLogger() *NoopLogger { | ||
| return &NoopLogger{} | ||
| } | ||
|
|
||
| func (l *NoopLogger) Level() Level { | ||
| return DebugLevel | ||
| } | ||
|
|
||
| func (l *NoopLogger) Log(_ context.Context, _ Level, _ string, _ ...slog.Attr) { | ||
| // No-op | ||
| } | ||
|
|
||
| func (l *NoopLogger) Debug(_ string, _ ...slog.Attr) { | ||
| // No-op | ||
| } | ||
|
|
||
| func (l *NoopLogger) Info(_ string, _ ...slog.Attr) { | ||
| // No-op | ||
| } | ||
|
|
||
| func (l *NoopLogger) Warn(_ string, _ ...slog.Attr) { | ||
| // No-op | ||
| } | ||
|
|
||
| func (l *NoopLogger) Error(_ string, _ ...slog.Attr) { | ||
| // No-op | ||
| } | ||
|
|
||
| func (l *NoopLogger) Fatal(_ string, _ ...slog.Attr) { | ||
| // No-op | ||
| } | ||
|
|
||
| func (l *NoopLogger) WithFields(_ ...slog.Attr) Logger { | ||
| return l | ||
| } | ||
|
|
||
| func (l *NoopLogger) WithError(_ error) Logger { | ||
| return l | ||
| } | ||
|
|
||
| func (l *NoopLogger) Named(_ string) Logger { | ||
| return l | ||
| } | ||
|
|
||
| func (l *NoopLogger) WithLevel(_ Level) Logger { | ||
| return l | ||
| } | ||
|
|
||
| func (l *NoopLogger) Sync() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The NoopLogger implementation lacks test coverage. While it's a simple no-op implementation, tests should verify that it correctly implements the Logger interface and that all methods are safe to call without side effects. Similar packages in this repository have corresponding test files.
| // | ||
| // This is useful when trying to get the string values for Level and mapping level in other external libraries. For example: | ||
| // ``` | ||
| // trace.SetLogLevel(kvp.String("loglevel", log.DebugLevel.String()) |
There was a problem hiding this comment.
The example code in the comment contains a syntax error. It shows kvp.String("loglevel", log.DebugLevel.String()) but should likely be demonstrating a valid function call.
| // trace.SetLogLevel(kvp.String("loglevel", log.DebugLevel.String()) | |
| // trace.SetLogLevel(kvp.String("loglevel", log.DebugLevel.String())) |
| WarnLevel = Level{"warn"} | ||
| // ErrorLevel causes all logs of level error or more severe to be logged | ||
| ErrorLevel = Level{"error"} | ||
| // FatalLevel causes only logs of level fatal to be logged |
There was a problem hiding this comment.
The comment for FatalLevel states "causes only logs of level fatal to be logged" which is inconsistent with other level comments. This suggests filtering that would suppress all other logs, but FatalLevel should be described as "causes all logs of level fatal to be logged" or "causes only fatal severity logs to be logged" to maintain consistency with the pattern of the other levels.
| // FatalLevel causes only logs of level fatal to be logged | |
| // FatalLevel causes all logs of level fatal to be logged |
| type Exporters struct { | ||
| Logger log.Logger | ||
| } | ||
|
|
||
| func NewExporters(logger log.Logger) *Exporters { | ||
| return &Exporters{ |
There was a problem hiding this comment.
The struct name "Exporters" is unclear for a type that currently only contains a Logger. The term "exporters" typically refers to components that send telemetry data to external systems (like metrics/traces exporters in OpenTelemetry). Consider renaming to something more descriptive like "Observability", "ObservabilityContext", or "Telemetry" to better represent its purpose. If this struct is intended to hold multiple exporters in the future (metrics, traces, etc.), add a comment documenting that intent.
| type Exporters struct { | |
| Logger log.Logger | |
| } | |
| func NewExporters(logger log.Logger) *Exporters { | |
| return &Exporters{ | |
| // Observability holds observability-related components such as logging. | |
| // Additional fields (e.g., metrics, tracing) can be added here in the future. | |
| type Observability struct { | |
| Logger log.Logger | |
| } | |
| func NewObservability(logger log.Logger) *Observability { | |
| return &Observability{ |
| return l.level | ||
| } | ||
|
|
||
| func (l *SlogLogger) Log(ctx context.Context, level Level, msg string, fields ...slog.Attr) { |
There was a problem hiding this comment.
The level field in SlogLogger is stored but never used for filtering log messages. The Log method ignores l.level and passes all messages to the underlying slog.Logger regardless of their severity. This means the level parameter in NewSlogLogger and the Level() method serve no practical purpose. Either implement level filtering in the Log method to respect the configured level, or remove the unused level field if filtering is meant to be handled by the underlying slog.Logger configuration.
| func (l *SlogLogger) Log(ctx context.Context, level Level, msg string, fields ...slog.Attr) { | |
| func (l *SlogLogger) Log(ctx context.Context, level Level, msg string, fields ...slog.Attr) { | |
| // Only log messages at or above the configured level | |
| if level < l.level { | |
| return | |
| } |
This allows us to have a deeper observability stack in the MCP server, particularly for surfacing logging in the remote MCP server.