Initial OSS logging adapter for http

[mattdholloway]

Summary

This pull request introduces a new observability layer to the codebase, providing a unified interface for logging and metrics. The main changes include the addition of a backend-agnostic Metrics interface (with a no-op implementation), a new Exporters type to bundle logger and metrics for dependency injection, and the integration of these observability primitives into the server and dependency injection systems.

Why

Fixes #

What changed

Observability Abstraction and Integration

• Added the new observability package, which provides the Exporters interface for bundling a structured logger (slog.Logger) and a metrics client, along with a default implementation and test coverage. [1] [2]
• Introduced the metrics subpackage, defining a backend-agnostic Metrics interface and a NoopMetrics implementation for use in tests or as a default, with comprehensive tests. [1] [2] [3]

Dependency Injection and Interface Updates

• Updated the ToolDependencies interface to add Logger(ctx) and Metrics(ctx) methods, and implemented these in both BaseDeps and RequestDeps using the new observability.Exporters bundle. [1] [2] [3] [4] [5]
• Modified constructors (NewBaseDeps, NewRequestDeps) and their usages throughout the codebase and tests to accept and propagate an observability.Exporters parameter. [1] [2] [3] [4] [5] [6]

Test and Stub Updates

• Updated all affected tests and stubs to account for the new observability dependency, ensuring backward compatibility and test coverage. [1] [2] [3] [4] [5] [6] [7] [8] [9]

Imports and Boilerplate

• Added necessary imports for the new observability and metrics packages in all relevant files. [1] [2] [3] [4]

These changes collectively provide a unified and extensible foundation for observability (logging and metrics) throughout the codebase, making it easier to integrate different backends, improve testability, and maintain consistent patterns.

MCP impact

• No tool or API changes
• Tool schema or behavior changed
• New tool added

Prompts tested (tool changes only)

Security / limits

• No security or limits impact
• Auth / permissions considered
• Data exposure, filtering, or token/size limits considered

Tool renaming

• I am renaming tools as part of this PR (e.g. a part of a consolidation effort)
  • I have added the new tool aliases in deprecated_tool_aliases.go
• I am not renaming tools as part of this PR

Note: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.

Lint & tests

• Linted locally with ./script/lint
• Tested locally with ./script/test

Docs

• Not needed
• Updated (README / docs / examples)

[Copilot]

Pull request overview

This PR introduces a new pkg/observability layer (logger + metrics adapters) and wires it into the GitHub tool dependency construction for both HTTP and stdio server paths.

Changes:

• Added pkg/observability with backend-agnostic log.Logger / metrics.Metrics interfaces plus noop + slog adapters and tests.
• Extended pkg/github dependency plumbing to provide Logger() / Metrics() on ToolDependencies, and to accept a *slog.Logger in NewBaseDeps / NewRequestDeps.
• Threaded the HTTP server’s slog.Logger into request deps and the stdio server’s cfg.Logger into base deps.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/observability/observability.go	Defines Exporters interface and basic implementation.
pkg/observability/observability_test.go	Unit tests for NewExporters.
pkg/observability/log/logger.go	Adds backend-agnostic logger interface.
pkg/observability/log/level.go	Adds log level type/constants.
pkg/observability/log/field.go	Adds structured field type + helpers.
pkg/observability/log/noop_adapter.go	No-op logger implementation.
pkg/observability/log/slog_adapter.go	slog-backed logger adapter.
pkg/observability/log/slog_adapter_test.go	Tests for SlogLogger.
pkg/observability/metrics/metrics.go	Adds backend-agnostic metrics interface.
pkg/observability/metrics/noop_adapter.go	No-op metrics implementation.
pkg/observability/metrics/noop_adapter_test.go	Tests for NoopMetrics.
pkg/observability/metrics/slog_adapter.go	slog-backed metrics adapter.
pkg/observability/metrics/slog_adapter_test.go	Tests for SlogMetrics.
pkg/github/dependencies.go	Adds observability to ToolDependencies; updates constructors to accept *slog.Logger.
pkg/github/dependencies_test.go	Updates constructor calls for new signature.
pkg/github/feature_flags_test.go	Updates constructor calls for new signature.
pkg/github/dynamic_tools_test.go	Updates constructor calls for new signature.
pkg/github/server_test.go	Updates test deps to provide Logger/Metrics.
pkg/http/server.go	Passes HTTP server logger into NewRequestDeps.
internal/ghmcp/server.go	Passes cfg.Logger into NewBaseDeps.

Comments suppressed due to low confidence (3)

pkg/github/dependencies.go:145

• NewBaseDeps’ signature now requires a *slog.Logger, which is a breaking change for downstream callers. Consider preserving the existing constructor (old signature) and adding a new constructor/option for observability (e.g., NewBaseDepsWithLogger or functional options) to avoid forcing updates for all consumers.

	flags FeatureFlags,
	contentWindowSize int,
	featureChecker inventory.FeatureFlagChecker,
	logger *slog.Logger,
) *BaseDeps {

pkg/github/dependencies.go:296

• NewRequestDeps’ signature now requires a *slog.Logger, which is a breaking change for downstream callers. Consider keeping the old signature and adding an alternate constructor/option for supplying a logger to avoid a hard API break.

	t translations.TranslationHelperFunc,
	contentWindowSize int,
	featureChecker inventory.FeatureFlagChecker,
	logger *slog.Logger,
) *RequestDeps {

pkg/github/server_test.go:76

• stubDeps.Metrics ignores the provided ctx and calls the exporter with context.Background(), which can hide context-dependent metrics behavior in tests. Pass the incoming ctx through to s.obsv.Metrics(ctx) for consistency.

func (s stubDeps) Metrics(_ context.Context) mcpMetrics.Metrics {
	if s.obsv != nil {
		return s.obsv.Metrics(context.Background())
	}
	return mcpMetrics.NewNoopMetrics()

[Copilot]

The default observability/exporters initialization logic is duplicated in NewBaseDeps and NewRequestDeps. Consider extracting a small helper (e.g., newDefaultExporters(logger *slog.Logger) observability.Exporters) to keep behavior consistent and avoid future drift.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

pkg/github/dependencies.go:151

• Observability exporter initialization is duplicated in both NewBaseDeps and NewRequestDeps. Consider extracting a small helper (e.g., build exporters from *slog.Logger) so future changes (adding real metrics, changing default level, etc.) don’t need to be made in multiple places.

	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())
	}

[Copilot]

WithError logs err.Error() as a string, which discards the original error value (wrapping/unwrapping, error type, and any structured formatting a handler might provide). Prefer attaching the error itself (or reusing the Err(err) field helper) so downstream handlers can render/encode errors consistently.

Suggested change

	logger: l.logger.With("error", err.Error()),
	logger: l.logger.With("error", err),

[Copilot]

Fatal currently calls panic(...). If this logger is ever used in server code, a fatal log will crash the process (and may bypass graceful shutdown / deferred cleanup). Consider aligning semantics with common logging expectations (e.g., os.Exit(1) after logging/sync, or removing Fatal from the interface and returning errors instead).

[omgitsads]

Looking good, but let's just firm up the interface so that there is always a logger instance available and we don't have to do nil checks at usage.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

pkg/github/dependencies.go:202

• BaseDeps.Metrics returns nil when Obsv is nil. Similar to the logger, this makes ToolDependencies.Metrics(ctx) unsafe to call and pushes nil checks onto every caller. Consider defaulting to a noop metrics implementation when Obsv is nil (or make Obsv required).

func (d BaseDeps) Metrics(ctx context.Context) metrics.Metrics {
	if d.Obsv == nil {
		return nil
	}
	return d.Obsv.Metrics(ctx)

pkg/github/dependencies.go:422

• RequestDeps.Metrics dereferences d.obsv without guarding against nil. If NewRequestDeps is called with a nil obsv, this will panic; consider defaulting obsv in the constructor or returning a noop metrics implementation when obsv is nil.

// Metrics implements ToolDependencies.
func (d *RequestDeps) Metrics(ctx context.Context) metrics.Metrics {
	return d.obsv.Metrics(ctx)
}

[Copilot]

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.

[mattdholloway]

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

[Copilot]

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]

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]

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]

@mattdholloway I've opened a new pull request, #2264, to work on those changes. Once the pull request is ready, I'll request review from you.