Conversation
…to add-logging-stack-v2
…to add-logging-stack-v2
There was a problem hiding this comment.
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/observabilitywith backend-agnosticlog.Logger/metrics.Metricsinterfaces plus noop +slogadapters and tests. - Extended
pkg/githubdependency plumbing to provideLogger()/Metrics()onToolDependencies, and to accept a*slog.LoggerinNewBaseDeps/NewRequestDeps. - Threaded the HTTP server’s
slog.Loggerinto request deps and the stdio server’scfg.Loggerinto 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()
| 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()) |
There was a problem hiding this comment.
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.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The slog-based metrics adapter was never used — OSS always uses NoopMetrics and the remote server has its own DataDog-backed adapter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
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
NewBaseDepsandNewRequestDeps. 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())
}
| 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...) | ||
| } |
There was a problem hiding this comment.
SlogLogger stores a level but it is never used to filter log emission (and the convenience methods always log at fixed levels). As a result, NewSlogLogger(..., InfoLevel) does not actually suppress debug logs if the underlying slog handler allows them, and WithLevel(...) only changes Level() without changing behavior. Consider enforcing l.level inside Log (drop messages below the configured minimum) or remove the level field/WithLevel API and rely solely on the underlying slog handler’s level configuration.
| return l | ||
| } | ||
| return &SlogLogger{ | ||
| logger: l.logger.With("error", err.Error()), |
There was a problem hiding this comment.
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.
| logger: l.logger.With("error", err.Error()), | |
| logger: l.logger.With("error", err), |
| func (l *SlogLogger) Fatal(msg string, fields ...Field) { | ||
| l.Log(context.Background(), FatalLevel, msg, fields...) | ||
| _ = l.Sync() | ||
| panic("fatal: " + msg) | ||
| } |
There was a problem hiding this comment.
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).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
Why
Fixes #
What changed
Observability and Logging Infrastructure
Logger) and supporting types (Field,Level) in thepkg/observability/logpackage, providing structured, backend-agnostic logging primitives. [1] [2] [3]NoopLogger) and an adapter for Go'sslog(SlogLogger), allowing flexible logger injection and usage. [1] [2]Dependency Injection and Configuration
ToolDependencies,BaseDeps, andRequestDepsto include logger and metrics clients, and updated their constructors to accept and initialize a logger. This enables tools and server code to access observability components via dependency injection. [1] [2] [3] [4] [5] [6] [7] [8] [9]Server Integration
internal/ghmcp/server.go,pkg/http/server.go) to pass the logger into dependency constructors, ensuring observability is available throughout server components. [1] [2]Testing and Stub Updates
Imports and Package Organization
MCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
deprecated_tool_aliases.goNote: 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
./script/lint./script/testDocs