diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 5c4e7f6f1..5dfaf596c 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -18,6 +18,8 @@ import ( "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/lockdown" mcplog "github.com/github/github-mcp-server/pkg/log" + "github.com/github/github-mcp-server/pkg/observability" + "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" @@ -116,6 +118,10 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se featureChecker := createFeatureChecker(cfg.EnabledFeatures) // Create dependencies for tool handlers + obs, err := observability.NewExporters(cfg.Logger, metrics.NewNoopMetrics()) + if err != nil { + return nil, fmt.Errorf("failed to create observability exporters: %w", err) + } deps := github.NewBaseDeps( clients.rest, clients.gql, @@ -128,6 +134,7 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se }, cfg.ContentWindowSize, featureChecker, + obs, ) // Build and register the tool/resource/prompt inventory inventoryBuilder := github.NewInventory(cfg.Translator). diff --git a/pkg/github/context_tools_test.go b/pkg/github/context_tools_test.go index 392501985..39f2058be 100644 --- a/pkg/github/context_tools_test.go +++ b/pkg/github/context_tools_test.go @@ -96,9 +96,10 @@ func Test_GetMe(t *testing.T) { t.Run(tc.name, func(t *testing.T) { var deps ToolDependencies if tc.clientErr != "" { - deps = stubDeps{clientFn: stubClientFnErr(tc.clientErr)} + deps = stubDeps{clientFn: stubClientFnErr(tc.clientErr), obsv: stubExporters()} } else { - deps = BaseDeps{Client: github.NewClient(tc.mockedClient)} + obs := stubExporters() + deps = BaseDeps{Client: github.NewClient(tc.mockedClient), Obsv: obs} } handler := serverTool.Handler(deps) @@ -304,7 +305,7 @@ func Test_GetTeams(t *testing.T) { { name: "getting client fails", makeDeps: func() ToolDependencies { - return stubDeps{clientFn: stubClientFnErr("expected test error")} + return stubDeps{clientFn: stubClientFnErr("expected test error"), obsv: stubExporters()} }, requestArgs: map[string]any{}, expectToolError: true, @@ -315,6 +316,7 @@ func Test_GetTeams(t *testing.T) { makeDeps: func() ToolDependencies { return BaseDeps{ Client: github.NewClient(httpClientUserFails()), + Obsv: stubExporters(), } }, requestArgs: map[string]any{}, @@ -327,6 +329,7 @@ func Test_GetTeams(t *testing.T) { return stubDeps{ clientFn: stubClientFnFromHTTP(httpClientWithUser()), gqlClientFn: stubGQLClientFnErr("GraphQL client error"), + obsv: stubExporters(), } }, requestArgs: map[string]any{}, @@ -469,7 +472,7 @@ func Test_GetTeamMembers(t *testing.T) { }, { name: "getting GraphQL client fails", - deps: stubDeps{gqlClientFn: stubGQLClientFnErr("GraphQL client error")}, + deps: stubDeps{gqlClientFn: stubGQLClientFnErr("GraphQL client error"), obsv: stubExporters()}, requestArgs: map[string]any{ "org": "testorg", "team_slug": "testteam", diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index f966c531e..57c6133a8 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,8 @@ 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" + "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 +97,14 @@ type ToolDependencies interface { // IsFeatureEnabled checks if a feature flag is enabled. IsFeatureEnabled(ctx context.Context, flagName string) bool + + // Logger returns the structured logger, optionally enriched with + // request-scoped data from ctx. Integrators provide their own slog.Handler + // to control where logs are sent. + Logger(ctx context.Context) *slog.Logger + + // Metrics returns the metrics client + Metrics(ctx context.Context) metrics.Metrics } // BaseDeps is the standard implementation of ToolDependencies for the local server. @@ -113,6 +124,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,6 +142,7 @@ func NewBaseDeps( flags FeatureFlags, contentWindowSize int, featureChecker inventory.FeatureFlagChecker, + obsv observability.Exporters, ) *BaseDeps { return &BaseDeps{ Client: client, @@ -138,6 +153,7 @@ func NewBaseDeps( Flags: flags, ContentWindowSize: contentWindowSize, featureChecker: featureChecker, + Obsv: obsv, } } @@ -170,6 +186,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(_ context.Context) *slog.Logger { + return d.Obsv.Logger() +} + +// Metrics implements ToolDependencies. +func (d BaseDeps) Metrics(ctx context.Context) metrics.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 +273,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,6 +287,7 @@ func NewRequestDeps( t translations.TranslationHelperFunc, contentWindowSize int, featureChecker inventory.FeatureFlagChecker, + obsv observability.Exporters, ) *RequestDeps { return &RequestDeps{ apiHosts: apiHosts, @@ -267,6 +297,7 @@ func NewRequestDeps( T: t, ContentWindowSize: contentWindowSize, featureChecker: featureChecker, + obsv: obsv, } } @@ -374,6 +405,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(_ context.Context) *slog.Logger { + return d.obsv.Logger() +} + +// Metrics implements ToolDependencies. +func (d *RequestDeps) Metrics(ctx context.Context) metrics.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..1d747cae4 100644 --- a/pkg/github/dependencies_test.go +++ b/pkg/github/dependencies_test.go @@ -3,13 +3,21 @@ package github_test import ( "context" "errors" + "log/slog" "testing" "github.com/github/github-mcp-server/pkg/github" + "github.com/github/github-mcp-server/pkg/observability" + "github.com/github/github-mcp-server/pkg/observability/metrics" "github.com/github/github-mcp-server/pkg/translations" "github.com/stretchr/testify/assert" ) +func testExporters() observability.Exporters { + obs, _ := observability.NewExporters(slog.New(slog.DiscardHandler), metrics.NewNoopMetrics()) + return obs +} + func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) { t.Parallel() @@ -28,6 +36,7 @@ func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize checker, // featureChecker + testExporters(), ) // Test enabled flag @@ -52,6 +61,7 @@ func TestIsFeatureEnabled_WithoutChecker(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize nil, // featureChecker (nil) + testExporters(), ) // Should return false when checker is nil @@ -76,6 +86,7 @@ func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize checker, // featureChecker + testExporters(), ) // Should return false for empty flag name @@ -100,6 +111,7 @@ func TestIsFeatureEnabled_CheckerError(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize checker, // featureChecker + testExporters(), ) // 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..ec559099e 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, stubExporters()), T: translations.NullTranslationHelper, } diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index 2f0a435c9..0f08c4f12 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, + stubExporters(), ) // Get the tool and its handler @@ -166,6 +167,7 @@ func TestHelloWorld_ConditionalBehavior_Config(t *testing.T) { FeatureFlags{InsidersMode: tt.insidersMode}, 0, nil, + stubExporters(), ) // Get the tool and its handler diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index 325900732..bf29ed132 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -5,11 +5,14 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "net/http" "testing" "time" "github.com/github/github-mcp-server/pkg/lockdown" + "github.com/github/github-mcp-server/pkg/observability" + "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" @@ -30,6 +33,7 @@ type stubDeps struct { t translations.TranslationHelperFunc flags FeatureFlags contentWindowSize int + obsv observability.Exporters } func (s stubDeps) GetClient(ctx context.Context) (*gogithub.Client, error) { @@ -60,8 +64,21 @@ 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) *slog.Logger { + return s.obsv.Logger() +} +func (s stubDeps) Metrics(ctx context.Context) metrics.Metrics { + return s.obsv.Metrics(ctx) +} // Helper functions to create stub client functions for error testing + +// stubExporters returns a discard-logger + noop-metrics Exporters for tests. +func stubExporters() observability.Exporters { + obs, _ := observability.NewExporters(slog.New(slog.DiscardHandler), metrics.NewNoopMetrics()) + return obs +} + func stubClientFnFromHTTP(httpClient *http.Client) func(context.Context) (*gogithub.Client, error) { return func(_ context.Context) (*gogithub.Client, error) { return gogithub.NewClient(httpClient), nil @@ -125,7 +142,7 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) { InsidersMode: false, } - deps := stubDeps{} + deps := stubDeps{obsv: stubExporters()} // Build inventory inv, err := NewInventory(cfg.Translator). diff --git a/pkg/http/server.go b/pkg/http/server.go index 872303940..55aed1c61 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -17,6 +17,8 @@ import ( "github.com/github/github-mcp-server/pkg/http/oauth" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/lockdown" + "github.com/github/github-mcp-server/pkg/observability" + "github.com/github/github-mcp-server/pkg/observability/metrics" "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/translations" "github.com/github/github-mcp-server/pkg/utils" @@ -106,6 +108,11 @@ func RunHTTPServer(cfg ServerConfig) error { featureChecker := createHTTPFeatureChecker() + obs, err := observability.NewExporters(logger, metrics.NewNoopMetrics()) + if err != nil { + return fmt.Errorf("failed to create observability exporters: %w", err) + } + deps := github.NewRequestDeps( apiHost, cfg.Version, @@ -114,6 +121,7 @@ func RunHTTPServer(cfg ServerConfig) error { t, cfg.ContentWindowSize, featureChecker, + obs, ) // Initialize the global tool scope map 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_sink.go b/pkg/observability/metrics/noop_sink.go new file mode 100644 index 000000000..4ce9e337d --- /dev/null +++ b/pkg/observability/metrics/noop_sink.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_sink_test.go b/pkg/observability/metrics/noop_sink_test.go new file mode 100644 index 000000000..21d3dccd6 --- /dev/null +++ b/pkg/observability/metrics/noop_sink_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/observability.go b/pkg/observability/observability.go new file mode 100644 index 000000000..3741b05c7 --- /dev/null +++ b/pkg/observability/observability.go @@ -0,0 +1,46 @@ +package observability + +import ( + "context" + "errors" + "log/slog" + + "github.com/github/github-mcp-server/pkg/observability/metrics" +) + +// Exporters bundles observability primitives (logger + metrics) for dependency injection. +// The logger is Go's stdlib *slog.Logger — integrators provide their own slog.Handler. +type Exporters interface { + Logger() *slog.Logger + Metrics(context.Context) metrics.Metrics +} + +type exporters struct { + logger *slog.Logger + metrics metrics.Metrics +} + +// NewExporters creates an Exporters bundle. Pass a configured *slog.Logger +// (with whatever slog.Handler you need) and a Metrics implementation. +// Neither may be nil; use slog.New(slog.DiscardHandler) and metrics.NewNoopMetrics() +// if logging or metrics are unwanted. +func NewExporters(logger *slog.Logger, m metrics.Metrics) (Exporters, error) { + if logger == nil { + return nil, errors.New("logger must not be nil: use slog.New(slog.DiscardHandler) to discard logs") + } + if m == nil { + return nil, errors.New("metrics must not be nil: use metrics.NewNoopMetrics() to discard metrics") + } + return &exporters{ + logger: logger, + metrics: m, + }, nil +} + +func (e *exporters) Logger() *slog.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..c8949fdbd --- /dev/null +++ b/pkg/observability/observability_test.go @@ -0,0 +1,46 @@ +package observability + +import ( + "context" + "log/slog" + "testing" + + "github.com/github/github-mcp-server/pkg/observability/metrics" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewExporters(t *testing.T) { + logger := slog.Default() + m := metrics.NewNoopMetrics() + exp, err := NewExporters(logger, m) + ctx := context.Background() + + require.NoError(t, err) + assert.NotNil(t, exp) + assert.Equal(t, logger, exp.Logger()) + assert.Equal(t, m, exp.Metrics(ctx)) +} + +func TestNewExporters_WithNilLogger(t *testing.T) { + _, err := NewExporters(nil, metrics.NewNoopMetrics()) + require.Error(t, err) + assert.Contains(t, err.Error(), "logger must not be nil") +} + +func TestNewExporters_WithNilMetrics(t *testing.T) { + _, err := NewExporters(slog.New(slog.DiscardHandler), nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "metrics must not be nil") +} + +func TestNewExporters_WithDiscardLogger(t *testing.T) { + logger := slog.New(slog.DiscardHandler) + m := metrics.NewNoopMetrics() + exp, err := NewExporters(logger, m) + + require.NoError(t, err) + assert.NotNil(t, exp) + assert.Equal(t, logger, exp.Logger()) + assert.Equal(t, m, exp.Metrics(context.Background())) +}