diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 6f5ba4e45..5bd013f9a 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -128,6 +128,7 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se }, cfg.ContentWindowSize, featureChecker, + cfg.Logger, ) // Build and register the tool/resource/prompt inventory inventoryBuilder := github.NewInventory(cfg.Translator). diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index f966c531e..5e46fdc3d 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log/slog" "net/http" "os" @@ -11,6 +12,9 @@ import ( "github.com/github/github-mcp-server/pkg/http/transport" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/lockdown" + "github.com/github/github-mcp-server/pkg/observability" + obsvLog "github.com/github/github-mcp-server/pkg/observability/log" + obsvMetrics "github.com/github/github-mcp-server/pkg/observability/metrics" "github.com/github/github-mcp-server/pkg/raw" "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/translations" @@ -94,6 +98,12 @@ type ToolDependencies interface { // IsFeatureEnabled checks if a feature flag is enabled. IsFeatureEnabled(ctx context.Context, flagName string) bool + + // Logger returns the logger + Logger(ctx context.Context) obsvLog.Logger + + // Metrics returns the metrics client + Metrics(ctx context.Context) obsvMetrics.Metrics } // BaseDeps is the standard implementation of ToolDependencies for the local server. @@ -113,6 +123,9 @@ type BaseDeps struct { // Feature flag checker for runtime checks featureChecker inventory.FeatureFlagChecker + + // Observability exporters (includes logger) + Obsv observability.Exporters } // Compile-time assertion to verify that BaseDeps implements the ToolDependencies interface. @@ -128,7 +141,14 @@ func NewBaseDeps( flags FeatureFlags, contentWindowSize int, featureChecker inventory.FeatureFlagChecker, + logger *slog.Logger, ) *BaseDeps { + var obsv observability.Exporters + if logger != nil { + obsv = observability.NewExporters(obsvLog.NewSlogLogger(logger, obsvLog.InfoLevel), obsvMetrics.NewNoopMetrics()) + } else { + obsv = observability.NewExporters(obsvLog.NewNoopLogger(), obsvMetrics.NewNoopMetrics()) + } return &BaseDeps{ Client: client, GQLClient: gqlClient, @@ -138,6 +158,7 @@ func NewBaseDeps( Flags: flags, ContentWindowSize: contentWindowSize, featureChecker: featureChecker, + Obsv: obsv, } } @@ -170,6 +191,16 @@ func (d BaseDeps) GetFlags(_ context.Context) FeatureFlags { return d.Flags } // GetContentWindowSize implements ToolDependencies. func (d BaseDeps) GetContentWindowSize() int { return d.ContentWindowSize } +// Logger implements ToolDependencies. +func (d BaseDeps) Logger(ctx context.Context) obsvLog.Logger { + return d.Obsv.Logger(ctx) +} + +// Metrics implements ToolDependencies. +func (d BaseDeps) Metrics(ctx context.Context) obsvMetrics.Metrics { + return d.Obsv.Metrics(ctx) +} + // IsFeatureEnabled checks if a feature flag is enabled. // Returns false if the feature checker is nil, flag name is empty, or an error occurs. // This allows tools to conditionally change behavior based on feature flags. @@ -247,6 +278,9 @@ type RequestDeps struct { // Feature flag checker for runtime checks featureChecker inventory.FeatureFlagChecker + + // Observability exporters (includes logger) + obsv observability.Exporters } // NewRequestDeps creates a RequestDeps with the provided clients and configuration. @@ -258,7 +292,14 @@ func NewRequestDeps( t translations.TranslationHelperFunc, contentWindowSize int, featureChecker inventory.FeatureFlagChecker, + logger *slog.Logger, ) *RequestDeps { + var obsv observability.Exporters + if logger != nil { + obsv = observability.NewExporters(obsvLog.NewSlogLogger(logger, obsvLog.InfoLevel), obsvMetrics.NewNoopMetrics()) + } else { + obsv = observability.NewExporters(obsvLog.NewNoopLogger(), obsvMetrics.NewNoopMetrics()) + } return &RequestDeps{ apiHosts: apiHosts, version: version, @@ -267,6 +308,7 @@ func NewRequestDeps( T: t, ContentWindowSize: contentWindowSize, featureChecker: featureChecker, + obsv: obsv, } } @@ -374,6 +416,16 @@ func (d *RequestDeps) GetFlags(ctx context.Context) FeatureFlags { // GetContentWindowSize implements ToolDependencies. func (d *RequestDeps) GetContentWindowSize() int { return d.ContentWindowSize } +// Logger implements ToolDependencies. +func (d *RequestDeps) Logger(ctx context.Context) obsvLog.Logger { + return d.obsv.Logger(ctx) +} + +// Metrics implements ToolDependencies. +func (d *RequestDeps) Metrics(ctx context.Context) obsvMetrics.Metrics { + return d.obsv.Metrics(ctx) +} + // IsFeatureEnabled checks if a feature flag is enabled. func (d *RequestDeps) IsFeatureEnabled(ctx context.Context, flagName string) bool { if d.featureChecker == nil || flagName == "" { diff --git a/pkg/github/dependencies_test.go b/pkg/github/dependencies_test.go index d13160d4c..a26e20c83 100644 --- a/pkg/github/dependencies_test.go +++ b/pkg/github/dependencies_test.go @@ -28,6 +28,7 @@ func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize checker, // featureChecker + nil, // logger ) // Test enabled flag @@ -52,6 +53,7 @@ func TestIsFeatureEnabled_WithoutChecker(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize nil, // featureChecker (nil) + nil, // logger ) // Should return false when checker is nil @@ -76,6 +78,7 @@ func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize checker, // featureChecker + nil, // logger ) // Should return false for empty flag name @@ -100,6 +103,7 @@ func TestIsFeatureEnabled_CheckerError(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize checker, // featureChecker + nil, // logger ) // Should return false and log error (not crash) diff --git a/pkg/github/dynamic_tools_test.go b/pkg/github/dynamic_tools_test.go index 3e63c5d7b..6e3d221dc 100644 --- a/pkg/github/dynamic_tools_test.go +++ b/pkg/github/dynamic_tools_test.go @@ -136,7 +136,7 @@ func TestDynamicTools_EnableToolset(t *testing.T) { deps := DynamicToolDependencies{ Server: server, Inventory: reg, - ToolDeps: NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0, nil), + ToolDeps: NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0, nil, nil), T: translations.NullTranslationHelper, } diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index 2f0a435c9..2149c9f49 100644 --- a/pkg/github/feature_flags_test.go +++ b/pkg/github/feature_flags_test.go @@ -104,6 +104,7 @@ func TestHelloWorld_ConditionalBehavior_Featureflag(t *testing.T) { FeatureFlags{}, 0, checker, + nil, ) // Get the tool and its handler @@ -166,6 +167,7 @@ func TestHelloWorld_ConditionalBehavior_Config(t *testing.T) { FeatureFlags{InsidersMode: tt.insidersMode}, 0, nil, + nil, ) // Get the tool and its handler diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index 2b99cab12..d475e3815 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -10,6 +10,9 @@ import ( "time" "github.com/github/github-mcp-server/pkg/lockdown" + "github.com/github/github-mcp-server/pkg/observability" + mcpLog "github.com/github/github-mcp-server/pkg/observability/log" + mcpMetrics "github.com/github/github-mcp-server/pkg/observability/metrics" "github.com/github/github-mcp-server/pkg/raw" "github.com/github/github-mcp-server/pkg/translations" gogithub "github.com/google/go-github/v82/github" @@ -29,6 +32,7 @@ type stubDeps struct { t translations.TranslationHelperFunc flags FeatureFlags contentWindowSize int + obsv observability.Exporters } func (s stubDeps) GetClient(ctx context.Context) (*gogithub.Client, error) { @@ -59,6 +63,18 @@ func (s stubDeps) GetT() translations.TranslationHelperFunc { return s. func (s stubDeps) GetFlags(_ context.Context) FeatureFlags { return s.flags } func (s stubDeps) GetContentWindowSize() int { return s.contentWindowSize } func (s stubDeps) IsFeatureEnabled(_ context.Context, _ string) bool { return false } +func (s stubDeps) Logger(_ context.Context) mcpLog.Logger { + if s.obsv != nil { + return s.obsv.Logger(context.Background()) + } + return mcpLog.NewNoopLogger() +} +func (s stubDeps) Metrics(_ context.Context) mcpMetrics.Metrics { + if s.obsv != nil { + return s.obsv.Metrics(context.Background()) + } + return mcpMetrics.NewNoopMetrics() +} // Helper functions to create stub client functions for error testing func stubClientFnFromHTTP(httpClient *http.Client) func(context.Context) (*gogithub.Client, error) { diff --git a/pkg/http/server.go b/pkg/http/server.go index 7397e54a8..efdde12aa 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -114,6 +114,7 @@ func RunHTTPServer(cfg ServerConfig) error { t, cfg.ContentWindowSize, featureChecker, + logger, ) // Initialize the global tool scope map diff --git a/pkg/observability/log/field.go b/pkg/observability/log/field.go new file mode 100644 index 000000000..402098d08 --- /dev/null +++ b/pkg/observability/log/field.go @@ -0,0 +1,29 @@ +package log + +import ( + "fmt" + "time" +) + +// Field is a key-value pair for structured logging. +// This type is backend-agnostic — adapters convert it to their native field type. +type Field struct { + Key string + Value any +} + +// Convenience constructors for common field types. + +func String(key, value string) Field { return Field{Key: key, Value: value} } +func Int(key string, value int) Field { return Field{Key: key, Value: value} } +func Int64(key string, value int64) Field { return Field{Key: key, Value: value} } +func Float64(key string, value float64) Field { return Field{Key: key, Value: value} } +func Bool(key string, value bool) Field { return Field{Key: key, Value: value} } +func Err(err error) Field { return Field{Key: "error", Value: err} } +func Duration(key string, value time.Duration) Field { return Field{Key: key, Value: value} } +func Any(key string, value any) Field { return Field{Key: key, Value: value} } + +// Stringer returns the string representation of a Field. +func (f Field) String() string { + return fmt.Sprintf("%s=%v", f.Key, f.Value) +} diff --git a/pkg/observability/log/level.go b/pkg/observability/log/level.go new file mode 100644 index 000000000..39fdc6286 --- /dev/null +++ b/pkg/observability/log/level.go @@ -0,0 +1,23 @@ +package log + +type Level struct { + level string +} + +var ( + // DebugLevel causes all logs to be logged + DebugLevel = Level{"debug"} + // InfoLevel causes all logs of level info or more severe to be logged + InfoLevel = Level{"info"} + // WarnLevel causes all logs of level warn or more severe to be logged + 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 + FatalLevel = Level{"fatal"} +) + +// String returns the string representation for Level +func (l Level) String() string { + return l.level +} diff --git a/pkg/observability/log/logger.go b/pkg/observability/log/logger.go new file mode 100644 index 000000000..32232a7af --- /dev/null +++ b/pkg/observability/log/logger.go @@ -0,0 +1,20 @@ +package log + +import ( + "context" +) + +type Logger interface { + Log(ctx context.Context, level Level, msg string, fields ...Field) + Debug(msg string, fields ...Field) + Info(msg string, fields ...Field) + Warn(msg string, fields ...Field) + Error(msg string, fields ...Field) + Fatal(msg string, fields ...Field) + WithFields(fields ...Field) Logger + WithError(err error) Logger + Named(name string) Logger + WithLevel(level Level) Logger + Sync() error + Level() Level +} diff --git a/pkg/observability/log/noop_adapter.go b/pkg/observability/log/noop_adapter.go new file mode 100644 index 000000000..83dd38f12 --- /dev/null +++ b/pkg/observability/log/noop_adapter.go @@ -0,0 +1,61 @@ +package log + +import ( + "context" +) + +type NoopLogger struct{} + +var _ Logger = (*NoopLogger)(nil) + +func NewNoopLogger() *NoopLogger { + return &NoopLogger{} +} + +func (l *NoopLogger) Level() Level { + return DebugLevel +} + +func (l *NoopLogger) Log(_ context.Context, _ Level, _ string, _ ...Field) { + // No-op +} + +func (l *NoopLogger) Debug(_ string, _ ...Field) { + // No-op +} + +func (l *NoopLogger) Info(_ string, _ ...Field) { + // No-op +} + +func (l *NoopLogger) Warn(_ string, _ ...Field) { + // No-op +} + +func (l *NoopLogger) Error(_ string, _ ...Field) { + // No-op +} + +func (l *NoopLogger) Fatal(_ string, _ ...Field) { + // No-op +} + +func (l *NoopLogger) WithFields(_ ...Field) 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 +} diff --git a/pkg/observability/log/slog_adapter.go b/pkg/observability/log/slog_adapter.go new file mode 100644 index 000000000..b3e4d6616 --- /dev/null +++ b/pkg/observability/log/slog_adapter.go @@ -0,0 +1,119 @@ +package log + +import ( + "context" + "fmt" + "log/slog" +) + +type SlogLogger struct { + logger *slog.Logger + level Level +} + +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 ...Field) { + slogLevel := convertLevel(level) + l.logger.LogAttrs(ctx, slogLevel, msg, fieldsToSlogAttrs(fields)...) +} + +func (l *SlogLogger) Debug(msg string, fields ...Field) { + l.Log(context.Background(), DebugLevel, msg, fields...) +} + +func (l *SlogLogger) Info(msg string, fields ...Field) { + l.Log(context.Background(), InfoLevel, msg, fields...) +} + +func (l *SlogLogger) Warn(msg string, fields ...Field) { + l.Log(context.Background(), WarnLevel, msg, fields...) +} + +func (l *SlogLogger) Error(msg string, fields ...Field) { + l.Log(context.Background(), ErrorLevel, msg, fields...) +} + +func (l *SlogLogger) Fatal(msg string, fields ...Field) { + l.Log(context.Background(), FatalLevel, msg, fields...) + _ = l.Sync() + panic("fatal: " + msg) +} + +func (l *SlogLogger) WithFields(fields ...Field) Logger { + args := make([]any, 0, len(fields)*2) + for _, f := range fields { + args = append(args, f.Key, f.Value) + } + return &SlogLogger{ + logger: l.logger.With(args...), + level: l.level, + } +} + +func (l *SlogLogger) WithError(err error) Logger { + if err == nil { + return l + } + 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 { + return nil +} + +func fieldsToSlogAttrs(fields []Field) []slog.Attr { + attrs := make([]slog.Attr, 0, len(fields)) + for _, f := range fields { + attrs = append(attrs, slog.Any(f.Key, f.Value)) + } + return attrs +} + +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 + } +} + +// FormatValue formats any value as a string, used by adapters for display. +func FormatValue(v any) string { + return fmt.Sprintf("%v", v) +} diff --git a/pkg/observability/log/slog_adapter_test.go b/pkg/observability/log/slog_adapter_test.go new file mode 100644 index 000000000..14353e122 --- /dev/null +++ b/pkg/observability/log/slog_adapter_test.go @@ -0,0 +1,269 @@ +package log + +import ( + "bytes" + "context" + "errors" + "log/slog" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewSlogLogger(t *testing.T) { + slogger := slog.New(slog.NewTextHandler(&bytes.Buffer{}, nil)) + logger := NewSlogLogger(slogger, InfoLevel) + + assert.NotNil(t, logger) + assert.Equal(t, InfoLevel, logger.Level()) +} + +func TestSlogLogger_Level(t *testing.T) { + tests := []struct { + name string + level Level + }{ + {"debug", DebugLevel}, + {"info", InfoLevel}, + {"warn", WarnLevel}, + {"error", ErrorLevel}, + {"fatal", FatalLevel}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, nil)) + logger := NewSlogLogger(slogger, tt.level) + + assert.Equal(t, tt.level, logger.Level()) + }) + } +} + +func TestSlogLogger_ConvenienceMethods(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + logger := NewSlogLogger(slogger, DebugLevel) + + tests := []struct { + name string + logFunc func(string, ...Field) + level string + }{ + {"Debug", logger.Debug, "DEBUG"}, + {"Info", logger.Info, "INFO"}, + {"Warn", logger.Warn, "WARN"}, + {"Error", logger.Error, "ERROR"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + buf.Reset() + tt.logFunc("test message", String("key", "value")) + + output := buf.String() + assert.Contains(t, output, "test message") + assert.Contains(t, output, tt.level) + assert.Contains(t, output, "key=value") + }) + } +} + +func TestSlogLogger_Fatal(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + logger := NewSlogLogger(slogger, DebugLevel) + + assert.Panics(t, func() { + logger.Fatal("fatal message") + }) + + assert.Contains(t, buf.String(), "fatal message") +} + +func TestSlogLogger_WithFields(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + logger := NewSlogLogger(slogger, InfoLevel) + + loggerWithFields := logger.WithFields( + String("service", "test-service"), + Int("port", 8080), + ) + + loggerWithFields.Info("message with fields") + + output := buf.String() + assert.Contains(t, output, "message with fields") + assert.Contains(t, output, "service") + assert.Contains(t, output, "test-service") + assert.Contains(t, output, "port") + assert.Contains(t, output, "8080") + + buf.Reset() + logger.Info("message without fields") + output = buf.String() + assert.NotContains(t, output, "service") +} + +func TestSlogLogger_WithError(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + logger := NewSlogLogger(slogger, InfoLevel) + + testErr := errors.New("test error message") + loggerWithError := logger.WithError(testErr) + + loggerWithError.Error("operation failed") + + output := buf.String() + assert.Contains(t, output, "operation failed") + assert.Contains(t, output, "error") + assert.Contains(t, output, "test error message") +} + +func TestSlogLogger_Named(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + logger := NewSlogLogger(slogger, InfoLevel) + + namedLogger := logger.Named("my-component") + namedLogger.Info("component message") + + output := buf.String() + assert.Contains(t, output, "component message") + assert.Contains(t, output, "logger") + assert.Contains(t, output, "my-component") +} + +func TestSlogLogger_WithLevel(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + logger := NewSlogLogger(slogger, InfoLevel) + + debugLogger := logger.WithLevel(DebugLevel) + + assert.Equal(t, InfoLevel, logger.Level()) + assert.Equal(t, DebugLevel, debugLogger.Level()) +} + +func TestSlogLogger_Sync(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, nil)) + logger := NewSlogLogger(slogger, InfoLevel) + + err := logger.Sync() + assert.NoError(t, err) +} + +func TestSlogLogger_Chaining(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + logger := NewSlogLogger(slogger, DebugLevel) + + chainedLogger := logger. + Named("service"). + WithFields(String("version", "1.0")). + WithLevel(InfoLevel) + + chainedLogger.Info("chained message") + + output := buf.String() + assert.Contains(t, output, "chained message") + assert.Contains(t, output, "service") + assert.Contains(t, output, "version") + assert.Contains(t, output, "1.0") +} + +func TestConvertLevel(t *testing.T) { + tests := []struct { + name string + input Level + expected slog.Level + }{ + {"debug to slog debug", DebugLevel, slog.LevelDebug}, + {"info to slog info", InfoLevel, slog.LevelInfo}, + {"warn to slog warn", WarnLevel, slog.LevelWarn}, + {"error to slog error", ErrorLevel, slog.LevelError}, + {"fatal to slog error", FatalLevel, slog.LevelError}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := convertLevel(tt.input) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestConvertLevel_Unknown(t *testing.T) { + unknownLevel := Level{"unknown"} + result := convertLevel(unknownLevel) + assert.Equal(t, slog.LevelInfo, result) +} + +func TestSlogLogger_ImplementsInterface(_ *testing.T) { + var _ Logger = (*SlogLogger)(nil) +} + +func TestSlogLogger_LogWithContext(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + logger := NewSlogLogger(slogger, DebugLevel) + + ctx := context.Background() + logger.Log(ctx, InfoLevel, "context message", String("trace_id", "abc123")) + + output := buf.String() + assert.Contains(t, output, "context message") + assert.Contains(t, output, "trace_id") + assert.Contains(t, output, "abc123") +} + +func TestSlogLogger_WithFields_PreservesLevel(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + logger := NewSlogLogger(slogger, WarnLevel) + + withFields := logger.WithFields(String("key", "value")) + + assert.Equal(t, WarnLevel, withFields.Level()) + + withFields.Warn("should appear") + assert.Contains(t, buf.String(), "should appear") + assert.Contains(t, buf.String(), "key=value") +} + +func TestSlogLogger_WithError_NilSafe(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + logger := NewSlogLogger(slogger, InfoLevel) + + require.NotPanics(t, func() { + result := logger.WithError(nil) + assert.NotNil(t, result) + assert.Equal(t, logger, result) + }) +} + +func TestSlogLogger_MultipleFields(t *testing.T) { + buf := &bytes.Buffer{} + slogger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + logger := NewSlogLogger(slogger, DebugLevel) + + logger.Info("multi-field message", + String("string_field", "value"), + Int("int_field", 42), + Bool("bool_field", true), + Float64("float_field", 3.14), + ) + + output := buf.String() + assert.Contains(t, output, "multi-field message") + assert.Contains(t, output, "string_field") + assert.Contains(t, output, "int_field") + assert.Contains(t, output, "bool_field") + assert.Contains(t, output, "float_field") +} diff --git a/pkg/observability/metrics/metrics.go b/pkg/observability/metrics/metrics.go new file mode 100644 index 000000000..5e861b3e0 --- /dev/null +++ b/pkg/observability/metrics/metrics.go @@ -0,0 +1,13 @@ +package metrics + +import "time" + +// Metrics is a backend-agnostic interface for emitting metrics. +// Implementations can route to DataDog, log to slog, or discard (noop). +type Metrics interface { + Increment(key string, tags map[string]string) + Counter(key string, tags map[string]string, value int64) + Distribution(key string, tags map[string]string, value float64) + DistributionMs(key string, tags map[string]string, value time.Duration) + WithTags(tags map[string]string) Metrics +} diff --git a/pkg/observability/metrics/noop_adapter.go b/pkg/observability/metrics/noop_adapter.go new file mode 100644 index 000000000..7707a0aca --- /dev/null +++ b/pkg/observability/metrics/noop_adapter.go @@ -0,0 +1,19 @@ +package metrics + +import "time" + +// NoopMetrics is a no-op implementation of the Metrics interface. +type NoopMetrics struct{} + +var _ Metrics = (*NoopMetrics)(nil) + +// NewNoopMetrics returns a new NoopMetrics. +func NewNoopMetrics() *NoopMetrics { + return &NoopMetrics{} +} + +func (n *NoopMetrics) Increment(_ string, _ map[string]string) {} +func (n *NoopMetrics) Counter(_ string, _ map[string]string, _ int64) {} +func (n *NoopMetrics) Distribution(_ string, _ map[string]string, _ float64) {} +func (n *NoopMetrics) DistributionMs(_ string, _ map[string]string, _ time.Duration) {} +func (n *NoopMetrics) WithTags(_ map[string]string) Metrics { return n } diff --git a/pkg/observability/metrics/noop_adapter_test.go b/pkg/observability/metrics/noop_adapter_test.go new file mode 100644 index 000000000..21d3dccd6 --- /dev/null +++ b/pkg/observability/metrics/noop_adapter_test.go @@ -0,0 +1,42 @@ +package metrics + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestNoopMetrics_ImplementsInterface(_ *testing.T) { + var _ Metrics = (*NoopMetrics)(nil) +} + +func TestNoopMetrics_NoPanics(t *testing.T) { + m := NewNoopMetrics() + + assert.NotPanics(t, func() { + m.Increment("key", map[string]string{"a": "b"}) + m.Counter("key", map[string]string{"a": "b"}, 1) + m.Distribution("key", map[string]string{"a": "b"}, 1.5) + m.DistributionMs("key", map[string]string{"a": "b"}, time.Second) + }) +} + +func TestNoopMetrics_NilTags(t *testing.T) { + m := NewNoopMetrics() + + assert.NotPanics(t, func() { + m.Increment("key", nil) + m.Counter("key", nil, 1) + m.Distribution("key", nil, 1.5) + m.DistributionMs("key", nil, time.Second) + }) +} + +func TestNoopMetrics_WithTags(t *testing.T) { + m := NewNoopMetrics() + tagged := m.WithTags(map[string]string{"env": "prod"}) + + assert.NotNil(t, tagged) + assert.Equal(t, m, tagged) +} diff --git a/pkg/observability/metrics/slog_adapter.go b/pkg/observability/metrics/slog_adapter.go new file mode 100644 index 000000000..514cf01b9 --- /dev/null +++ b/pkg/observability/metrics/slog_adapter.go @@ -0,0 +1,58 @@ +package metrics + +import ( + "fmt" + "log/slog" + "maps" + "time" +) + +// SlogMetrics implements Metrics by logging metric emissions via slog. +// Useful for debugging metrics in local development. +type SlogMetrics struct { + logger *slog.Logger + tags map[string]string +} + +var _ Metrics = (*SlogMetrics)(nil) + +// NewSlogMetrics returns a new SlogMetrics that logs to the given slog.Logger. +func NewSlogMetrics(logger *slog.Logger) *SlogMetrics { + return &SlogMetrics{logger: logger} +} + +func (s *SlogMetrics) mergedTags(tags map[string]string) map[string]string { + if len(s.tags) == 0 { + return tags + } + if len(tags) == 0 { + return s.tags + } + merged := make(map[string]string, len(s.tags)+len(tags)) + maps.Copy(merged, s.tags) + maps.Copy(merged, tags) + return merged +} + +func (s *SlogMetrics) Increment(key string, tags map[string]string) { + s.logger.Debug("metric.increment", slog.String("key", key), slog.Any("tags", s.mergedTags(tags))) +} + +func (s *SlogMetrics) Counter(key string, tags map[string]string, value int64) { + s.logger.Debug("metric.counter", slog.String("key", key), slog.Int64("value", value), slog.Any("tags", s.mergedTags(tags))) +} + +func (s *SlogMetrics) Distribution(key string, tags map[string]string, value float64) { + s.logger.Debug("metric.distribution", slog.String("key", key), slog.Float64("value", value), slog.Any("tags", s.mergedTags(tags))) +} + +func (s *SlogMetrics) DistributionMs(key string, tags map[string]string, value time.Duration) { + s.logger.Debug("metric.distribution_ms", slog.String("key", key), slog.String("value", fmt.Sprintf("%dms", value.Milliseconds())), slog.Any("tags", s.mergedTags(tags))) +} + +func (s *SlogMetrics) WithTags(tags map[string]string) Metrics { + merged := make(map[string]string, len(s.tags)+len(tags)) + maps.Copy(merged, s.tags) + maps.Copy(merged, tags) + return &SlogMetrics{logger: s.logger, tags: merged} +} diff --git a/pkg/observability/metrics/slog_adapter_test.go b/pkg/observability/metrics/slog_adapter_test.go new file mode 100644 index 000000000..eff64d9a0 --- /dev/null +++ b/pkg/observability/metrics/slog_adapter_test.go @@ -0,0 +1,109 @@ +package metrics + +import ( + "bytes" + "log/slog" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestSlogMetrics_ImplementsInterface(_ *testing.T) { + var _ Metrics = (*SlogMetrics)(nil) +} + +func newTestSlogMetrics() (*SlogMetrics, *bytes.Buffer) { + buf := &bytes.Buffer{} + logger := slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + return NewSlogMetrics(logger), buf +} + +func TestSlogMetrics_Increment(t *testing.T) { + m, buf := newTestSlogMetrics() + m.Increment("req.count", map[string]string{"tool": "search"}) + + output := buf.String() + assert.Contains(t, output, "metric.increment") + assert.Contains(t, output, "req.count") + assert.Contains(t, output, "search") +} + +func TestSlogMetrics_Counter(t *testing.T) { + m, buf := newTestSlogMetrics() + m.Counter("api.calls", map[string]string{"status": "200"}, 5) + + output := buf.String() + assert.Contains(t, output, "metric.counter") + assert.Contains(t, output, "api.calls") + assert.Contains(t, output, "5") +} + +func TestSlogMetrics_Distribution(t *testing.T) { + m, buf := newTestSlogMetrics() + m.Distribution("latency", map[string]string{"endpoint": "/api"}, 42.5) + + output := buf.String() + assert.Contains(t, output, "metric.distribution") + assert.Contains(t, output, "latency") + assert.Contains(t, output, "42.5") +} + +func TestSlogMetrics_DistributionMs(t *testing.T) { + m, buf := newTestSlogMetrics() + m.DistributionMs("duration", map[string]string{"op": "fetch"}, 150*time.Millisecond) + + output := buf.String() + assert.Contains(t, output, "metric.distribution_ms") + assert.Contains(t, output, "duration") + assert.Contains(t, output, "150ms") +} + +func TestSlogMetrics_WithTags(t *testing.T) { + m, buf := newTestSlogMetrics() + tagged := m.WithTags(map[string]string{"env": "prod"}) + + tagged.Increment("req.count", map[string]string{"tool": "search"}) + + output := buf.String() + assert.Contains(t, output, "env") + assert.Contains(t, output, "prod") + assert.Contains(t, output, "search") +} + +func TestSlogMetrics_WithTags_Chaining(t *testing.T) { + m, buf := newTestSlogMetrics() + tagged := m.WithTags(map[string]string{"env": "prod"}).WithTags(map[string]string{"region": "us"}) + + tagged.Increment("req.count", nil) + + output := buf.String() + assert.Contains(t, output, "env") + assert.Contains(t, output, "prod") + assert.Contains(t, output, "region") + assert.Contains(t, output, "us") +} + +func TestSlogMetrics_WithTags_DoesNotMutateOriginal(t *testing.T) { + m, buf := newTestSlogMetrics() + _ = m.WithTags(map[string]string{"env": "prod"}) + + m.Increment("req.count", nil) + + output := buf.String() + assert.NotContains(t, output, "prod") +} + +func TestSlogMetrics_NilTags(t *testing.T) { + m, buf := newTestSlogMetrics() + + assert.NotPanics(t, func() { + m.Increment("key", nil) + m.Counter("key", nil, 1) + m.Distribution("key", nil, 1.5) + m.DistributionMs("key", nil, time.Second) + }) + + output := buf.String() + assert.Contains(t, output, "metric.increment") +} diff --git a/pkg/observability/observability.go b/pkg/observability/observability.go new file mode 100644 index 000000000..ce0f6e52c --- /dev/null +++ b/pkg/observability/observability.go @@ -0,0 +1,33 @@ +package observability + +import ( + "context" + + "github.com/github/github-mcp-server/pkg/observability/log" + "github.com/github/github-mcp-server/pkg/observability/metrics" +) + +type Exporters interface { + Logger(context.Context) log.Logger + Metrics(context.Context) metrics.Metrics +} + +type exporters struct { + logger log.Logger + metrics metrics.Metrics +} + +func NewExporters(logger log.Logger, metrics metrics.Metrics) Exporters { + return &exporters{ + logger: logger, + metrics: metrics, + } +} + +func (e *exporters) Logger(_ context.Context) log.Logger { + return e.logger +} + +func (e *exporters) Metrics(_ context.Context) metrics.Metrics { + return e.metrics +} diff --git a/pkg/observability/observability_test.go b/pkg/observability/observability_test.go new file mode 100644 index 000000000..e3f49f738 --- /dev/null +++ b/pkg/observability/observability_test.go @@ -0,0 +1,30 @@ +package observability + +import ( + "context" + "testing" + + "github.com/github/github-mcp-server/pkg/observability/log" + "github.com/github/github-mcp-server/pkg/observability/metrics" + "github.com/stretchr/testify/assert" +) + +func TestNewExporters(t *testing.T) { + logger := log.NewNoopLogger() + m := metrics.NewNoopMetrics() + exp := NewExporters(logger, m) + ctx := context.Background() + + assert.NotNil(t, exp) + assert.Equal(t, logger, exp.Logger(ctx)) + assert.Equal(t, m, exp.Metrics(ctx)) +} + +func TestExporters_WithNilLogger(t *testing.T) { + exp := NewExporters(nil, nil) + ctx := context.Background() + + assert.NotNil(t, exp) + assert.Nil(t, exp.Logger(ctx)) + assert.Nil(t, exp.Metrics(ctx)) +}