Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
a8bf0b0
initial logging stack for http
mattdholloway Feb 12, 2026
4a2f8ed
Merge branch 'main' of https://github.com/github/github-mcp-server in…
mattdholloway Feb 12, 2026
ba9d315
Merge branch 'main' of https://github.com/github/github-mcp-server in…
mattdholloway Feb 17, 2026
cbf99e7
add metrics adapter
mattdholloway Feb 18, 2026
cc5b8e7
Merge branch 'main' into add-logging-stack-v2
mattdholloway Feb 18, 2026
77f6e57
fix linter issues
mattdholloway Feb 18, 2026
3bbfaa0
make log fields generic
mattdholloway Feb 18, 2026
dd57d84
Merge branch 'main' into add-logging-stack-v2
mattdholloway Feb 25, 2026
2a20a2a
Update pkg/github/server_test.go
mattdholloway Feb 25, 2026
1648775
Merge branch 'main' into add-logging-stack-v2
mattdholloway Feb 25, 2026
7c5ea0a
Remove unused SlogMetrics adapter
mattdholloway Feb 25, 2026
121daa3
Update pkg/github/dependencies.go
mattdholloway Feb 26, 2026
39445b0
fmt
mattdholloway Feb 26, 2026
f7d3a75
Merge branch 'main' into add-logging-stack-v2
mattdholloway Feb 27, 2026
30b2952
Merge branch 'main' into add-logging-stack-v2
mattdholloway Mar 17, 2026
c0a68f5
change to use slog
mattdholloway Mar 18, 2026
457f64d
address feedback
mattdholloway Mar 26, 2026
f94b729
Merge branch 'main' into add-logging-stack-v2
mattdholloway Mar 26, 2026
e31ea3b
rename noop adapter to noop sink
mattdholloway Mar 26, 2026
095d9f2
Update pkg/http/server.go
mattdholloway Mar 26, 2026
1ae4a03
[WIP] [WIP] Address feedback on OSS logging adapter for http implemen…
Copilot Mar 26, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions internal/ghmcp/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand All @@ -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).
Expand Down
47 changes: 47 additions & 0 deletions pkg/github/dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@ import (
"context"
"errors"
"fmt"
"log/slog"
"net/http"
"os"

ghcontext "github.com/github/github-mcp-server/pkg/context"
"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"
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -128,6 +142,7 @@ func NewBaseDeps(
flags FeatureFlags,
contentWindowSize int,
featureChecker inventory.FeatureFlagChecker,
obsv observability.Exporters,
) *BaseDeps {
return &BaseDeps{
Client: client,
Expand All @@ -138,6 +153,7 @@ func NewBaseDeps(
Flags: flags,
ContentWindowSize: contentWindowSize,
featureChecker: featureChecker,
Obsv: obsv,
}
}

Expand Down Expand Up @@ -170,6 +186,22 @@ 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 {
if d.Obsv == nil {
return slog.New(slog.DiscardHandler)
}
return d.Obsv.Logger()
Comment on lines +190 to +194
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

BaseDeps.Logger returns nil when Obsv is nil. Since NewExporters enforces a non-nil logger and ToolDependencies.Logger(ctx) is expected to be callable, returning nil here is an inconsistent contract and invites nil-pointer panics at call sites. Consider returning a discard logger when Obsv is nil (or ensuring BaseDeps always has a non-nil Obsv).

This issue also appears on line 198 of the same file.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

}

// Metrics implements ToolDependencies.
func (d BaseDeps) Metrics(ctx context.Context) metrics.Metrics {
if d.Obsv == nil {
return metrics.NewNoopMetrics()
}
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.
Expand Down Expand Up @@ -247,6 +279,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.
Expand All @@ -258,6 +293,7 @@ func NewRequestDeps(
t translations.TranslationHelperFunc,
contentWindowSize int,
featureChecker inventory.FeatureFlagChecker,
obsv observability.Exporters,
) *RequestDeps {
return &RequestDeps{
apiHosts: apiHosts,
Expand All @@ -267,6 +303,7 @@ func NewRequestDeps(
T: t,
ContentWindowSize: contentWindowSize,
featureChecker: featureChecker,
obsv: obsv,
}
}

Expand Down Expand Up @@ -374,6 +411,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 {
Comment on lines +415 to +420
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

RequestDeps.Logger dereferences d.obsv without a nil check, but NewRequestDeps accepts obsv observability.Exporters and call sites/tests can pass nil. This can panic if obsv is not provided; consider making obsv required (and validating it) or defaulting it to discard/noop exporters in the constructor.

This issue also appears on line 419 of the same file.

Suggested change
func (d *RequestDeps) Logger(_ context.Context) *slog.Logger {
return d.obsv.Logger()
}
// Metrics implements ToolDependencies.
func (d *RequestDeps) Metrics(ctx context.Context) metrics.Metrics {
func (d *RequestDeps) Logger(_ context.Context) *slog.Logger {
if d.obsv == nil {
return slog.Default()
}
return d.obsv.Logger()
}
// Metrics implements ToolDependencies.
func (d *RequestDeps) Metrics(ctx context.Context) metrics.Metrics {
if d.obsv == nil {
return nil
}

Copilot uses AI. Check for mistakes.
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 == "" {
Expand Down
4 changes: 4 additions & 0 deletions pkg/github/dependencies_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) {
github.FeatureFlags{},
0, // contentWindowSize
checker, // featureChecker
nil, // obsv
)

// Test enabled flag
Expand All @@ -52,6 +53,7 @@ func TestIsFeatureEnabled_WithoutChecker(t *testing.T) {
github.FeatureFlags{},
0, // contentWindowSize
nil, // featureChecker (nil)
nil, // obsv
)

// Should return false when checker is nil
Expand All @@ -76,6 +78,7 @@ func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) {
github.FeatureFlags{},
0, // contentWindowSize
checker, // featureChecker
nil, // obsv
)

// Should return false for empty flag name
Expand All @@ -100,6 +103,7 @@ func TestIsFeatureEnabled_CheckerError(t *testing.T) {
github.FeatureFlags{},
0, // contentWindowSize
checker, // featureChecker
nil, // obsv
)

// Should return false and log error (not crash)
Expand Down
2 changes: 1 addition & 1 deletion pkg/github/dynamic_tools_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}

Expand Down
2 changes: 2 additions & 0 deletions pkg/github/feature_flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ func TestHelloWorld_ConditionalBehavior_Featureflag(t *testing.T) {
FeatureFlags{},
0,
checker,
nil,
)

// Get the tool and its handler
Expand Down Expand Up @@ -166,6 +167,7 @@ func TestHelloWorld_ConditionalBehavior_Config(t *testing.T) {
FeatureFlags{InsidersMode: tt.insidersMode},
0,
nil,
nil,
)

// Get the tool and its handler
Expand Down
16 changes: 16 additions & 0 deletions pkg/github/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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) {
Expand Down Expand Up @@ -60,6 +64,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) *slog.Logger {
if s.obsv != nil {
return s.obsv.Logger()
}
return slog.New(slog.DiscardHandler)
}
func (s stubDeps) Metrics(ctx context.Context) metrics.Metrics {
if s.obsv != nil {
return s.obsv.Metrics(ctx)
}
return metrics.NewNoopMetrics()
}

// Helper functions to create stub client functions for error testing
func stubClientFnFromHTTP(httpClient *http.Client) func(context.Context) (*gogithub.Client, error) {
Expand Down
8 changes: 8 additions & 0 deletions pkg/http/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand All @@ -114,6 +121,7 @@ func RunHTTPServer(cfg ServerConfig) error {
t,
cfg.ContentWindowSize,
featureChecker,
obs,
)

// Initialize the global tool scope map
Expand Down
13 changes: 13 additions & 0 deletions pkg/observability/metrics/metrics.go
Original file line number Diff line number Diff line change
@@ -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
}
19 changes: 19 additions & 0 deletions pkg/observability/metrics/noop_sink.go
Original file line number Diff line number Diff line change
@@ -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 }
42 changes: 42 additions & 0 deletions pkg/observability/metrics/noop_sink_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
42 changes: 42 additions & 0 deletions pkg/observability/observability.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
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.
// 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")
}
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

NewExporters allows metrics to be nil, which means Exporters.Metrics(...) will return nil. Since callers will typically expect a usable metrics client (even if noop), consider defaulting nil metrics to a NoopMetrics implementation or returning an error when metrics is nil.

Suggested change
}
}
if metrics == nil {
return nil, errors.New("metrics must not be nil; provide a no-op metrics implementation if metrics are disabled")
}

Copilot uses AI. Check for mistakes.
return &exporters{
logger: logger,
metrics: metrics,
Comment on lines +26 to +32
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The metrics parameter name shadows the imported metrics package name, which makes this function harder to read/maintain. Renaming the parameter (e.g., m) avoids the shadowing.

Suggested change
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,
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")
}
return &exporters{
logger: logger,
metrics: m,

Copilot uses AI. Check for mistakes.
}, nil
}

func (e *exporters) Logger() *slog.Logger {
return e.logger
}

func (e *exporters) Metrics(_ context.Context) metrics.Metrics {
return e.metrics
}
Loading
Loading