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>

Author: Copilot

internal/logger/common.go

@@ -194,21 +194,33 @@ 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:
 //   - Each set is a distinct public API with a different signature and set of callers.
 //   - 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
 //

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)

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")
 

internal/logger/markdown_logger.go

@@ -170,41 +170,34 @@ 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) {
 		logger.Log(level, category, format, args...)
 	})
 }
 
-// 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

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...)

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) {

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

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

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.

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