From 36500a37319b6ca4b01707977bb3fb3b76a3c6b8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:31:06 +0000 Subject: [PATCH 1/2] Initial plan From b1972318acb9c398c9f499b1c52ff43d4e975c06 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:44:02 +0000 Subject: [PATCH 2/2] refactor: deduplicate tracer fallback and simplify logger helpers Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/73f6ab54-aba0-4958-aa14-8eec3780a37e Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- internal/logger/common.go | 20 ++++++++++++++++---- internal/logger/file_logger.go | 12 ------------ internal/logger/helper_functions_test.go | 4 ++-- internal/logger/markdown_logger.go | 19 ++++++------------- internal/logger/server_file_logger.go | 2 +- internal/proxy/handler.go | 5 +---- internal/server/tool_registry.go | 2 ++ internal/server/unified.go | 5 +---- internal/tracing/provider.go | 8 ++++++++ internal/tracing/provider_test.go | 14 ++++++++++++++ 10 files changed, 51 insertions(+), 40 deletions(-) diff --git a/internal/logger/common.go b/internal/logger/common.go index 876d0c86..59d1cbe3 100644 --- a/internal/logger/common.go +++ b/internal/logger/common.go @@ -194,7 +194,7 @@ import ( // The three sets and their internal helpers are: // // file_logger.go LogInfo / LogWarn / LogError / LogDebug → logWithLevel -// markdown_logger.go LogInfoMd / LogWarnMd / LogErrorMd / LogDebugMd → logWithMarkdownLevel +// markdown_logger.go LogInfoMd / LogWarnMd / LogErrorMd / LogDebugMd → logWithMarkdown // server_file_logger.go LogInfoWithServer / ... / LogDebugWithServer → logWithLevelAndServer // // This pattern is intentionally kept across the three files because: @@ -202,13 +202,25 @@ import ( // - The one-liner wrappers are trivial and unlikely to diverge. // - Go lacks the metaprogramming to eliminate them without sacrificing readability. // -// The shared logFuncs map (file_logger.go) centralises the LogLevel → log-function -// mapping so that the internal helpers (logWithMarkdownLevel, logWithLevelAndServer) +// The shared logFuncs map below centralises the LogLevel → log-function +// mapping so that the internal helpers (logWithMarkdown, logWithLevelAndServer) // do not need their own switch-on-level blocks. // // When adding a new LogLevel constant (e.g., LogLevelTrace): -// 1. Add a new entry to the logFuncs map in file_logger.go. +// 1. Add a new entry to the logFuncs map below. // 2. Add a new LogTrace wrapper to each of the three files above. +// +// logFuncs maps each LogLevel to its corresponding global log function. +// This eliminates repeated switch-on-level blocks in logWithMarkdown +// (markdown_logger.go) and logWithLevelAndServer (server_file_logger.go). +// When adding a new LogLevel constant, add a corresponding entry here so +// that all dispatch sites automatically support the new level. +var logFuncs = map[LogLevel]func(string, string, ...interface{}){ + LogLevelInfo: LogInfo, + LogLevelWarn: LogWarn, + LogLevelError: LogError, + LogLevelDebug: LogDebug, +} // Global Logger RWMutex Access Pattern // diff --git a/internal/logger/file_logger.go b/internal/logger/file_logger.go index 79a2bd44..6d84552b 100644 --- a/internal/logger/file_logger.go +++ b/internal/logger/file_logger.go @@ -133,18 +133,6 @@ func LogDebug(category, format string, args ...interface{}) { logWithLevel(LogLevelDebug, category, format, args...) } -// logFuncs maps each LogLevel to its corresponding global log function. -// This eliminates repeated switch-on-level blocks in logWithMarkdownLevel -// (markdown_logger.go) and logWithLevelAndServer (server_file_logger.go). -// When adding a new LogLevel constant, add a corresponding entry here so -// that all dispatch sites automatically support the new level. -var logFuncs = map[LogLevel]func(string, string, ...interface{}){ - LogLevelInfo: LogInfo, - LogLevelWarn: LogWarn, - LogLevelError: LogError, - LogLevelDebug: LogDebug, -} - // CloseGlobalLogger closes the global file logger func CloseGlobalLogger() error { return closeGlobalLogger(&globalLoggerMu, &globalFileLogger) diff --git a/internal/logger/helper_functions_test.go b/internal/logger/helper_functions_test.go index fa33de46..eed70818 100644 --- a/internal/logger/helper_functions_test.go +++ b/internal/logger/helper_functions_test.go @@ -99,9 +99,9 @@ func TestLogWithLevelAndServer(t *testing.T) { assert.Contains(t, unifiedLog, "Debug message via server helper") } -// TestLogWithMarkdownLevel verifies the logWithMarkdownLevel helper function +// TestLogWithMarkdown verifies the logWithMarkdown helper function // correctly handles both regular and markdown logging -func TestLogWithMarkdownLevel(t *testing.T) { +func TestLogWithMarkdown(t *testing.T) { tmpDir := t.TempDir() logDir := filepath.Join(tmpDir, "logs") diff --git a/internal/logger/markdown_logger.go b/internal/logger/markdown_logger.go index 4c3653aa..a715e391 100644 --- a/internal/logger/markdown_logger.go +++ b/internal/logger/markdown_logger.go @@ -170,9 +170,9 @@ func (ml *MarkdownLogger) Log(level LogLevel, category, format string, args ...i // logWithMarkdown is a helper that logs to both regular and markdown loggers. // It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking // and nil-checking for the markdown logger access. -func logWithMarkdown(level LogLevel, regularLogFunc func(string, string, ...interface{}), category, format string, args ...interface{}) { +func logWithMarkdown(level LogLevel, category, format string, args ...interface{}) { // Log to regular logger - regularLogFunc(category, format, args...) + logFuncs[level](category, format, args...) // Log to markdown logger using withGlobalLogger helper withGlobalLogger(&globalMarkdownMu, &globalMarkdownLogger, func(logger *MarkdownLogger) { @@ -180,31 +180,24 @@ func logWithMarkdown(level LogLevel, regularLogFunc func(string, string, ...inte }) } -// logWithMarkdownLevel is a helper that reduces code duplication for markdown logging at different levels. -// It uses the logFuncs map (file_logger.go) to look up the regular log function for the given level, -// eliminating repeated switch-on-level patterns across LogInfoMd, LogWarnMd, LogErrorMd, and LogDebugMd. -func logWithMarkdownLevel(level LogLevel, category, format string, args ...interface{}) { - logWithMarkdown(level, logFuncs[level], category, format, args...) -} - // LogInfoMd logs to both regular and markdown loggers func LogInfoMd(category, format string, args ...interface{}) { - logWithMarkdownLevel(LogLevelInfo, category, format, args...) + logWithMarkdown(LogLevelInfo, category, format, args...) } // LogWarnMd logs to both regular and markdown loggers func LogWarnMd(category, format string, args ...interface{}) { - logWithMarkdownLevel(LogLevelWarn, category, format, args...) + logWithMarkdown(LogLevelWarn, category, format, args...) } // LogErrorMd logs to both regular and markdown loggers func LogErrorMd(category, format string, args ...interface{}) { - logWithMarkdownLevel(LogLevelError, category, format, args...) + logWithMarkdown(LogLevelError, category, format, args...) } // LogDebugMd logs to both regular and markdown loggers func LogDebugMd(category, format string, args ...interface{}) { - logWithMarkdownLevel(LogLevelDebug, category, format, args...) + logWithMarkdown(LogLevelDebug, category, format, args...) } // CloseMarkdownLogger closes the global markdown logger diff --git a/internal/logger/server_file_logger.go b/internal/logger/server_file_logger.go index 990e3f47..5ed47c58 100644 --- a/internal/logger/server_file_logger.go +++ b/internal/logger/server_file_logger.go @@ -141,7 +141,7 @@ func (sfl *ServerFileLogger) Close() error { // logWithLevelAndServer is a helper that reduces code duplication for per-server logging at different levels. // It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking and nil-checking, // eliminating repeated patterns across LogInfoWithServer, LogWarnWithServer, LogErrorWithServer, and LogDebugWithServer. -// It uses the logFuncs map (file_logger.go) for the unified log dispatch, avoiding a repeated switch-on-level block. +// It uses the logFuncs map (common.go) for the unified log dispatch, avoiding a repeated switch-on-level block. func logWithLevelAndServer(serverID string, level LogLevel, category, format string, args ...interface{}) { withGlobalLogger(&globalServerLoggerMu, &globalServerFileLogger, func(logger *ServerFileLogger) { logger.Log(serverID, level, category, format, args...) diff --git a/internal/proxy/handler.go b/internal/proxy/handler.go index 94bc3678..6a3ee594 100644 --- a/internal/proxy/handler.go +++ b/internal/proxy/handler.go @@ -40,10 +40,7 @@ type proxyHandler struct { // getTracer returns the cached tracer if set, otherwise falls back to the global tracer. func (h *proxyHandler) getTracer() oteltrace.Tracer { - if h.tracer != nil { - return h.tracer - } - return tracing.Tracer() + return tracing.GetCachedOrGlobal(h.tracer) } func (h *proxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { diff --git a/internal/server/tool_registry.go b/internal/server/tool_registry.go index fe8ed123..fe98c3b0 100644 --- a/internal/server/tool_registry.go +++ b/internal/server/tool_registry.go @@ -350,6 +350,8 @@ func (us *UnifiedServer) callSysServer(toolName string) (interface{}, error) { } func marshalAndSanitizeForLog(value interface{}) string { + // Best-effort logging helper: if marshaling fails, we intentionally keep logging + // a sanitized empty string rather than surfacing an additional logging-only error. resultJSON, _ := json.Marshal(value) return sanitize.SanitizeString(string(resultJSON)) } diff --git a/internal/server/unified.go b/internal/server/unified.go index 401f6610..966ae763 100644 --- a/internal/server/unified.go +++ b/internal/server/unified.go @@ -124,10 +124,7 @@ type UnifiedServer struct { // getTracer returns the cached tracer if set, otherwise falls back to the global tracer. func (us *UnifiedServer) getTracer() oteltrace.Tracer { - if us.tracer != nil { - return us.tracer - } - return tracing.Tracer() + return tracing.GetCachedOrGlobal(us.tracer) } // NewUnified creates a new unified MCP server diff --git a/internal/tracing/provider.go b/internal/tracing/provider.go index a3a0994e..fd0f3ee7 100644 --- a/internal/tracing/provider.go +++ b/internal/tracing/provider.go @@ -306,6 +306,14 @@ func Tracer() trace.Tracer { return otel.Tracer(instrumentationName) } +// GetCachedOrGlobal returns cached if non-nil, otherwise falls back to the global tracer. +func GetCachedOrGlobal(cached trace.Tracer) trace.Tracer { + if cached != nil { + return cached + } + return Tracer() +} + // ParentContext returns a context carrying the W3C remote parent span context // from the configured traceId and spanId (spec §4.1.3.6). // Exported for use at startup to build the root span's parent context. diff --git a/internal/tracing/provider_test.go b/internal/tracing/provider_test.go index d6066631..88a874cb 100644 --- a/internal/tracing/provider_test.go +++ b/internal/tracing/provider_test.go @@ -92,6 +92,20 @@ func TestTracer_ReturnsNonNil(t *testing.T) { assert.NotNil(t, tr) } +func TestGetCachedOrGlobal_WithCachedTracer_ReturnsCached(t *testing.T) { + cached := noop.NewTracerProvider().Tracer("cached") + assert.Equal(t, cached, tracing.GetCachedOrGlobal(cached)) +} + +func TestGetCachedOrGlobal_WithNilTracer_ReturnsGlobal(t *testing.T) { + ctx := context.Background() + provider, err := tracing.InitProvider(ctx, nil) + require.NoError(t, err) + defer provider.Shutdown(ctx) + + assert.NotNil(t, tracing.GetCachedOrGlobal(nil)) +} + func TestInitProvider_SampleRateZero_UsesNeverSampler(t *testing.T) { ctx := context.Background()