address feedback

Author: mattdholloway

internal/ghmcp/server.go

@@ -19,6 +19,7 @@ import (
 	"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"
@@ -117,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,
@@ -129,7 +134,7 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se
 		},
 		cfg.ContentWindowSize,
 		featureChecker,
-		observability.NewExporters(cfg.Logger, nil),
+		obs,
 	)
 	// Build and register the tool/resource/prompt inventory
 	inventoryBuilder := github.NewInventory(cfg.Translator).

pkg/github/server_test.go

@@ -68,7 +68,7 @@ func (s stubDeps) Logger(_ context.Context) *slog.Logger {
 	if s.obsv != nil {
 		return s.obsv.Logger()
 	}
-	return nil
+	return slog.New(slog.DiscardHandler)
 }
 func (s stubDeps) Metrics(ctx context.Context) metrics.Metrics {
 	if s.obsv != nil {

pkg/http/server.go

@@ -107,6 +107,11 @@ func RunHTTPServer(cfg ServerConfig) error {
 
 	featureChecker := createHTTPFeatureChecker()
 
+	obs, err := observability.NewExporters(logger, nil)
+	if err != nil {
+		return fmt.Errorf("failed to create observability exporters: %w", err)
+	}
+
 	deps := github.NewRequestDeps(
 		apiHost,
 		cfg.Version,
@@ -115,7 +120,7 @@ func RunHTTPServer(cfg ServerConfig) error {
 		t,
 		cfg.ContentWindowSize,
 		featureChecker,
-		observability.NewExporters(logger, nil),
+		obs,
 	)
 
 	// Initialize the global tool scope map

pkg/observability/observability.go

@@ -2,6 +2,7 @@ package observability
 
 import (
 	"context"
+	"errors"
 	"log/slog"
 
 	"github.com/github/github-mcp-server/pkg/observability/metrics"
@@ -21,11 +22,15 @@ type exporters struct {
 
 // NewExporters creates an Exporters bundle. Pass a configured *slog.Logger
 // (with whatever slog.Handler you need) and a Metrics implementation.
-func NewExporters(logger *slog.Logger, metrics metrics.Metrics) Exporters {
+// The logger must not be nil; use slog.New(slog.DiscardHandler) if logging is unwanted.
+func NewExporters(logger *slog.Logger, metrics metrics.Metrics) (Exporters, error) {
+	if logger == nil {
+		return nil, errors.New("logger must not be nil: use slog.New(slog.DiscardHandler) to discard logs")
+	}
 	return &exporters{
 		logger:  logger,
 		metrics: metrics,
-	}
+	}, nil
 }
 
 func (e *exporters) Logger() *slog.Logger {

pkg/observability/observability_test.go

@@ -7,24 +7,32 @@ import (
 
 	"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 := NewExporters(logger, m)
+	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 TestExporters_WithNilLogger(t *testing.T) {
-	exp := NewExporters(nil, nil)
-	ctx := context.Background()
+func TestNewExporters_WithNilLogger(t *testing.T) {
+	_, err := NewExporters(nil, nil)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "logger must not be nil")
+}
 
+func TestNewExporters_WithDiscardLogger(t *testing.T) {
+	logger := slog.New(slog.DiscardHandler)
+	exp, err := NewExporters(logger, nil)
+
+	require.NoError(t, err)
 	assert.NotNil(t, exp)
-	assert.Nil(t, exp.Logger())
-	assert.Nil(t, exp.Metrics(ctx))
+	assert.Equal(t, logger, exp.Logger())
 }