Skip to content

Commit 971fb5a

Browse files
lpcoxlpcox
authored
Refactor tracer fallback + logger helper layout to remove duplication and tighten logging semantics (#4213)
Semantic clustering flagged four targeted refactor opportunities: duplicated tracer fallback logic, misplaced shared logger dispatch state, unnecessary markdown logging indirection, and unclear marshal-error handling in a logging helper. This PR consolidates those seams without changing external behavior. - **Tracer fallback deduplicated (`internal/tracing`, `internal/proxy`, `internal/server`)** - Added `tracing.GetCachedOrGlobal(cached trace.Tracer)` as the single fallback path. - Replaced duplicated `getTracer()` implementations in `proxyHandler` and `UnifiedServer` with one-line calls to the shared helper. - **`logFuncs` ownership moved to `common.go` (`internal/logger`)** - Relocated shared `LogLevel -> function` dispatch map from `file_logger.go` to `common.go`. - Co-located implementation with the existing design guidance and updated references to the new source location. - **Markdown logging helper flow simplified (`internal/logger/markdown_logger.go`)** - Removed `logWithMarkdownLevel` indirection. - Updated `logWithMarkdown` to resolve `logFuncs[level]` internally, reducing one helper layer and tightening call flow for `Log*Md` wrappers. - **Marshal helper intent made explicit (`internal/server/tool_registry.go`)** - Documented `marshalAndSanitizeForLog` as best-effort logging behavior when JSON marshaling fails (sanitized empty-string fallback by design). ```go // internal/tracing/provider.go func GetCachedOrGlobal(cached trace.Tracer) trace.Tracer { if cached != nil { return cached } return Tracer() } ``` > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build1164881766/b513/launcher.test /tmp/go-build1164881766/b513/launcher.test -test.testlogfile=/tmp/go-build1164881766/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1164881766/b437/vet.cfg _.a go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet -E mBfaT_lUZ 544961/b287/ x_amd64/vet -I . -imultiarch x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build3536398537/b513/launcher.test /tmp/go-build3536398537/b513/launcher.test -test.testlogfile=/tmp/go-build3536398537/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s list�� ithub-guard/rust-guard/target/de/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rust/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/link s Agent-Logs-Url: REDACTED ithub-guard/rust-guard/target/de/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rust/tmp/go-build3536398537/b507/guard.test Ld/symbols.o 437f known-linux-gnu/-bool known-linux-gnu/-buildtags know�� known-linux-gnu/-errorsas known-linux-gnu/-ifaceassert known-linux-gnu/-nilfunc known-linux-gnu/bash known-linux-gnu//usr/bin/runc known-linux-gnu/--version known-linux-gnu/-tests` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1164881766/b495/config.test /tmp/go-build1164881766/b495/config.test -test.testlogfile=/tmp/go-build1164881766/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1164881766/b395/vet.cfg /http2/ascii.go /http2/ciphers.go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet --de�� _.a zhqG-0L32 x_amd64/vet -I` (dns block) > - Triggering command: `/tmp/go-build3536398537/b495/config.test /tmp/go-build3536398537/b495/config.test -test.testlogfile=/tmp/go-build3536398537/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s lib/�� lib/rustlib/x86_/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rust-guard/target/debash lib/rustlib/x86_/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rust-guard/target/de--norc lib/rustlib/x86_/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rust-guard/target/de--noprofile lib/rustlib/x86_git lib/rustlib/x86_checkout lib/rustlib/x86_copilot/refactor-semantic-function-clustering lib/rustlib/x86_/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rust-guard/target/debug/deps/github_guard-57d41235e07a5585.53ehv5r2twmuqycv01jbjtq95.19dllou.rcgu.o lib/�� lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libminiz_oxide-2b6a8d2f6e1dc71b.rlib lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libadler2-39ffdbc27c978ccc.rlib &#34;CURL_CA_BUNDLE=//home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/echo &#34;&#34; c1063f.rlib .cfg` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build1164881766/b513/launcher.test /tmp/go-build1164881766/b513/launcher.test -test.testlogfile=/tmp/go-build1164881766/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1164881766/b437/vet.cfg _.a go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet -E mBfaT_lUZ 544961/b287/ x_amd64/vet -I . -imultiarch x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build3536398537/b513/launcher.test /tmp/go-build3536398537/b513/launcher.test -test.testlogfile=/tmp/go-build3536398537/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s list�� ithub-guard/rust-guard/target/de/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rust/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/link s Agent-Logs-Url: REDACTED ithub-guard/rust-guard/target/de/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rust/tmp/go-build3536398537/b507/guard.test Ld/symbols.o 437f known-linux-gnu/-bool known-linux-gnu/-buildtags know�� known-linux-gnu/-errorsas known-linux-gnu/-ifaceassert known-linux-gnu/-nilfunc known-linux-gnu/bash known-linux-gnu//usr/bin/runc known-linux-gnu/--version known-linux-gnu/-tests` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build1164881766/b513/launcher.test /tmp/go-build1164881766/b513/launcher.test -test.testlogfile=/tmp/go-build1164881766/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1164881766/b437/vet.cfg _.a go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet -E mBfaT_lUZ 544961/b287/ x_amd64/vet -I . -imultiarch x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build3536398537/b513/launcher.test /tmp/go-build3536398537/b513/launcher.test -test.testlogfile=/tmp/go-build3536398537/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s list�� ithub-guard/rust-guard/target/de/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rust/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/link s Agent-Logs-Url: REDACTED ithub-guard/rust-guard/target/de/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rust/tmp/go-build3536398537/b507/guard.test Ld/symbols.o 437f known-linux-gnu/-bool known-linux-gnu/-buildtags know�� known-linux-gnu/-errorsas known-linux-gnu/-ifaceassert known-linux-gnu/-nilfunc known-linux-gnu/bash known-linux-gnu//usr/bin/runc known-linux-gnu/--version known-linux-gnu/-tests` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1164881766/b522/mcp.test /tmp/go-build1164881766/b522/mcp.test -test.testlogfile=/tmp/go-build1164881766/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o cfg om/modelcontextprotocol/go-sdk@v1.5.0/mcp/cmd.go x_amd64/vet -p g/grpc/internal/--version -lang=go1.25 x_amd64/vet cfg /auth/header.go /auth/header_test.go x_amd64/vet -c /cobra /tmp/go-build945544961/b287/ dWtd0aB/LlpLOT-ZzUhjnWEpG9lp` (dns block) > - Triggering command: `/tmp/go-build3536398537/b522/mcp.test /tmp/go-build3536398537/b522/mcp.test -test.testlogfile=/tmp/go-build3536398537/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s know�� known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libobject-926daa94a00ee327.rlib known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libmemchr-48d5b0db80402653.rlib known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libaddr2line-3367f26bd486b29d.rlib known-linux-gnu/bash known-linux-gnu//usr/bin/runc known-linux-gnu/--version ive.12123747d8da05ed-cgu.00.rcgu.o ive.�� ive.12123747d8da05ed-cgu.02.rcgu.o ive.12123747d8da05ed-cgu.03.rcgu.o ive.12123747d8da05ed-cgu.04.rcgu.o ive.12123747d8da/usr/libexec/docker/cli-plugins/docker-buildx ive.12123747d8dadocker-cli-plugin-metadata ive.12123747d8da05ed-cgu.07.rcgu/tmp/go-build3867419718/b001/exe/a.out ive.12123747d8da05ed-cgu.08.rcgu-importcfg` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details>
2 parents 37dafa0 + b197231 commit 971fb5a

10 files changed

Lines changed: 51 additions & 40 deletions

File tree

‎internal/logger/common.go‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,21 +194,33 @@ import (
194194
// The three sets and their internal helpers are:
195195
//
196196
// file_logger.go LogInfo / LogWarn / LogError / LogDebug → logWithLevel
197-
// markdown_logger.go LogInfoMd / LogWarnMd / LogErrorMd / LogDebugMd → logWithMarkdownLevel
197+
// markdown_logger.go LogInfoMd / LogWarnMd / LogErrorMd / LogDebugMd → logWithMarkdown
198198
// server_file_logger.go LogInfoWithServer / ... / LogDebugWithServer → logWithLevelAndServer
199199
//
200200
// This pattern is intentionally kept across the three files because:
201201
// - Each set is a distinct public API with a different signature and set of callers.
202202
// - The one-liner wrappers are trivial and unlikely to diverge.
203203
// - Go lacks the metaprogramming to eliminate them without sacrificing readability.
204204
//
205-
// The shared logFuncs map (file_logger.go) centralises the LogLevel → log-function
206-
// mapping so that the internal helpers (logWithMarkdownLevel, logWithLevelAndServer)
205+
// The shared logFuncs map below centralises the LogLevel → log-function
206+
// mapping so that the internal helpers (logWithMarkdown, logWithLevelAndServer)
207207
// do not need their own switch-on-level blocks.
208208
//
209209
// When adding a new LogLevel constant (e.g., LogLevelTrace):
210-
// 1. Add a new entry to the logFuncs map in file_logger.go.
210+
// 1. Add a new entry to the logFuncs map below.
211211
// 2. Add a new LogTrace wrapper to each of the three files above.
212+
//
213+
// logFuncs maps each LogLevel to its corresponding global log function.
214+
// This eliminates repeated switch-on-level blocks in logWithMarkdown
215+
// (markdown_logger.go) and logWithLevelAndServer (server_file_logger.go).
216+
// When adding a new LogLevel constant, add a corresponding entry here so
217+
// that all dispatch sites automatically support the new level.
218+
var logFuncs = map[LogLevel]func(string, string, ...interface{}){
219+
LogLevelInfo: LogInfo,
220+
LogLevelWarn: LogWarn,
221+
LogLevelError: LogError,
222+
LogLevelDebug: LogDebug,
223+
}
212224

213225
// Global Logger RWMutex Access Pattern
214226
//

‎internal/logger/file_logger.go‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -133,18 +133,6 @@ func LogDebug(category, format string, args ...interface{}) {
133133
logWithLevel(LogLevelDebug, category, format, args...)
134134
}
135135

136-
// logFuncs maps each LogLevel to its corresponding global log function.
137-
// This eliminates repeated switch-on-level blocks in logWithMarkdownLevel
138-
// (markdown_logger.go) and logWithLevelAndServer (server_file_logger.go).
139-
// When adding a new LogLevel constant, add a corresponding entry here so
140-
// that all dispatch sites automatically support the new level.
141-
var logFuncs = map[LogLevel]func(string, string, ...interface{}){
142-
LogLevelInfo: LogInfo,
143-
LogLevelWarn: LogWarn,
144-
LogLevelError: LogError,
145-
LogLevelDebug: LogDebug,
146-
}
147-
148136
// CloseGlobalLogger closes the global file logger
149137
func CloseGlobalLogger() error {
150138
return closeGlobalLogger(&globalLoggerMu, &globalFileLogger)

‎internal/logger/helper_functions_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,9 @@ func TestLogWithLevelAndServer(t *testing.T) {
9999
assert.Contains(t, unifiedLog, "Debug message via server helper")
100100
}
101101

102-
// TestLogWithMarkdownLevel verifies the logWithMarkdownLevel helper function
102+
// TestLogWithMarkdown verifies the logWithMarkdown helper function
103103
// correctly handles both regular and markdown logging
104-
func TestLogWithMarkdownLevel(t *testing.T) {
104+
func TestLogWithMarkdown(t *testing.T) {
105105
tmpDir := t.TempDir()
106106
logDir := filepath.Join(tmpDir, "logs")
107107

‎internal/logger/markdown_logger.go‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -170,41 +170,34 @@ func (ml *MarkdownLogger) Log(level LogLevel, category, format string, args ...i
170170
// logWithMarkdown is a helper that logs to both regular and markdown loggers.
171171
// It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking
172172
// and nil-checking for the markdown logger access.
173-
func logWithMarkdown(level LogLevel, regularLogFunc func(string, string, ...interface{}), category, format string, args ...interface{}) {
173+
func logWithMarkdown(level LogLevel, category, format string, args ...interface{}) {
174174
// Log to regular logger
175-
regularLogFunc(category, format, args...)
175+
logFuncs[level](category, format, args...)
176176

177177
// Log to markdown logger using withGlobalLogger helper
178178
withGlobalLogger(&globalMarkdownMu, &globalMarkdownLogger, func(logger *MarkdownLogger) {
179179
logger.Log(level, category, format, args...)
180180
})
181181
}
182182

183-
// logWithMarkdownLevel is a helper that reduces code duplication for markdown logging at different levels.
184-
// It uses the logFuncs map (file_logger.go) to look up the regular log function for the given level,
185-
// eliminating repeated switch-on-level patterns across LogInfoMd, LogWarnMd, LogErrorMd, and LogDebugMd.
186-
func logWithMarkdownLevel(level LogLevel, category, format string, args ...interface{}) {
187-
logWithMarkdown(level, logFuncs[level], category, format, args...)
188-
}
189-
190183
// LogInfoMd logs to both regular and markdown loggers
191184
func LogInfoMd(category, format string, args ...interface{}) {
192-
logWithMarkdownLevel(LogLevelInfo, category, format, args...)
185+
logWithMarkdown(LogLevelInfo, category, format, args...)
193186
}
194187

195188
// LogWarnMd logs to both regular and markdown loggers
196189
func LogWarnMd(category, format string, args ...interface{}) {
197-
logWithMarkdownLevel(LogLevelWarn, category, format, args...)
190+
logWithMarkdown(LogLevelWarn, category, format, args...)
198191
}
199192

200193
// LogErrorMd logs to both regular and markdown loggers
201194
func LogErrorMd(category, format string, args ...interface{}) {
202-
logWithMarkdownLevel(LogLevelError, category, format, args...)
195+
logWithMarkdown(LogLevelError, category, format, args...)
203196
}
204197

205198
// LogDebugMd logs to both regular and markdown loggers
206199
func LogDebugMd(category, format string, args ...interface{}) {
207-
logWithMarkdownLevel(LogLevelDebug, category, format, args...)
200+
logWithMarkdown(LogLevelDebug, category, format, args...)
208201
}
209202

210203
// CloseMarkdownLogger closes the global markdown logger

‎internal/logger/server_file_logger.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ func (sfl *ServerFileLogger) Close() error {
141141
// logWithLevelAndServer is a helper that reduces code duplication for per-server logging at different levels.
142142
// It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking and nil-checking,
143143
// eliminating repeated patterns across LogInfoWithServer, LogWarnWithServer, LogErrorWithServer, and LogDebugWithServer.
144-
// It uses the logFuncs map (file_logger.go) for the unified log dispatch, avoiding a repeated switch-on-level block.
144+
// It uses the logFuncs map (common.go) for the unified log dispatch, avoiding a repeated switch-on-level block.
145145
func logWithLevelAndServer(serverID string, level LogLevel, category, format string, args ...interface{}) {
146146
withGlobalLogger(&globalServerLoggerMu, &globalServerFileLogger, func(logger *ServerFileLogger) {
147147
logger.Log(serverID, level, category, format, args...)

‎internal/proxy/handler.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,7 @@ type proxyHandler struct {
4040

4141
// getTracer returns the cached tracer if set, otherwise falls back to the global tracer.
4242
func (h *proxyHandler) getTracer() oteltrace.Tracer {
43-
if h.tracer != nil {
44-
return h.tracer
45-
}
46-
return tracing.Tracer()
43+
return tracing.GetCachedOrGlobal(h.tracer)
4744
}
4845

4946
func (h *proxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {

‎internal/server/tool_registry.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,8 @@ func (us *UnifiedServer) callSysServer(toolName string) (interface{}, error) {
350350
}
351351

352352
func marshalAndSanitizeForLog(value interface{}) string {
353+
// Best-effort logging helper: if marshaling fails, we intentionally keep logging
354+
// a sanitized empty string rather than surfacing an additional logging-only error.
353355
resultJSON, _ := json.Marshal(value)
354356
return sanitize.SanitizeString(string(resultJSON))
355357
}

‎internal/server/unified.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,10 +124,7 @@ type UnifiedServer struct {
124124

125125
// getTracer returns the cached tracer if set, otherwise falls back to the global tracer.
126126
func (us *UnifiedServer) getTracer() oteltrace.Tracer {
127-
if us.tracer != nil {
128-
return us.tracer
129-
}
130-
return tracing.Tracer()
127+
return tracing.GetCachedOrGlobal(us.tracer)
131128
}
132129

133130
// NewUnified creates a new unified MCP server

‎internal/tracing/provider.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,14 @@ func Tracer() trace.Tracer {
306306
return otel.Tracer(instrumentationName)
307307
}
308308

309+
// GetCachedOrGlobal returns cached if non-nil, otherwise falls back to the global tracer.
310+
func GetCachedOrGlobal(cached trace.Tracer) trace.Tracer {
311+
if cached != nil {
312+
return cached
313+
}
314+
return Tracer()
315+
}
316+
309317
// ParentContext returns a context carrying the W3C remote parent span context
310318
// from the configured traceId and spanId (spec §4.1.3.6).
311319
// Exported for use at startup to build the root span's parent context.

‎internal/tracing/provider_test.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,20 @@ func TestTracer_ReturnsNonNil(t *testing.T) {
9292
assert.NotNil(t, tr)
9393
}
9494

95+
func TestGetCachedOrGlobal_WithCachedTracer_ReturnsCached(t *testing.T) {
96+
cached := noop.NewTracerProvider().Tracer("cached")
97+
assert.Equal(t, cached, tracing.GetCachedOrGlobal(cached))
98+
}
99+
100+
func TestGetCachedOrGlobal_WithNilTracer_ReturnsGlobal(t *testing.T) {
101+
ctx := context.Background()
102+
provider, err := tracing.InitProvider(ctx, nil)
103+
require.NoError(t, err)
104+
defer provider.Shutdown(ctx)
105+
106+
assert.NotNil(t, tracing.GetCachedOrGlobal(nil))
107+
}
108+
95109
func TestInitProvider_SampleRateZero_UsesNeverSampler(t *testing.T) {
96110
ctx := context.Background()
97111

0 commit comments

Comments
 (0)