Refactor duplicated tracing and HTTP server setup paths in cmd/server packages#4048
Merged
Refactor duplicated tracing and HTTP server setup paths in cmd/server packages#4048
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/348fae3e-5b56-42bd-bd42-c82af76c53b5 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/348fae3e-5b56-42bd-bd42-c82af76c53b5 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Analyze and resolve duplicate code patterns identified in latest commit
Refactor duplicated tracing and HTTP server setup paths in cmd/server packages
Apr 18, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors duplicated tracing flag/provider lifecycle logic across the root and proxy commands, and centralizes repeated HTTP server construction in internal/server, with small focused tests to validate the new helpers.
Changes:
- Deduplicates OTLP tracing flag registration via a shared
registerTracingFlags(...)helper. - Deduplicates tracing provider init + shutdown-with-timeout via shared helpers reused by both
rootandproxy. - Deduplicates
http.Server{Addr, Handler}construction vianewHTTPServer(...)and adds targeted unit tests.
Show a summary per file
| File | Description |
|---|---|
| internal/server/transport.go | Uses shared newHTTPServer helper for MCP HTTP server creation. |
| internal/server/routed.go | Uses shared newHTTPServer helper for routed-mode HTTP server creation. |
| internal/server/http_server.go | Introduces shared newHTTPServer(addr, handler) helper. |
| internal/server/http_server_test.go | Adds unit test coverage for newHTTPServer. |
| internal/cmd/tracing_helpers.go | Introduces shared tracing flag registration + tracing provider init/shutdown helpers. |
| internal/cmd/tracing_helpers_test.go | Adds unit test verifying tracing flag defaults and flag-to-var binding. |
| internal/cmd/root.go | Switches to shared tracing provider init/shutdown helpers. |
| internal/cmd/proxy.go | Switches to shared tracing flag registration and tracing provider init/shutdown helpers. |
| internal/cmd/flags_tracing.go | Replaces duplicated tracing flag wiring with registerTracingFlags. |
| go.mod | Promotes github.com/spf13/pflag to a direct dependency (now imported directly). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 1
Comment on lines
+22
to
+27
| func initTracingProviderWithFallback(ctx context.Context, tracingCfg *config.TracingConfig, warnf func(format string, args ...any)) *tracing.Provider { | ||
| tracingProvider, err := tracing.InitProvider(ctx, tracingCfg) | ||
| if err != nil { | ||
| warnf("failed to initialize tracing provider: %v", err) | ||
| tracingProvider, _ = tracing.InitProvider(ctx, nil) | ||
| } |
Collaborator
|
@copilot address this review feedback #4048 (review) |
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/3029a801-65c4-417f-a446-690f567d84aa Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Contributor
Author
Addressed in 9894603. I updated the shared tracing init helper to accept a caller-provided warning format so each command preserves its original message text/capitalization ( |
This was referenced Apr 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Duplicate-code analysis identified repeated logic in three areas: OTLP flag registration, tracing provider lifecycle handling, and HTTP server construction. This PR factors those paths into shared helpers while preserving existing behavior in
rootandproxycommand flows.Tracing flags deduplication (
internal/cmd)registerTracingFlags(...)to centralize OTLP flag registration (otlp-endpoint,otlp-service-name,otlp-sample-rate).internal/cmd/flags_tracing.gointernal/cmd/proxy.goTracing provider lifecycle deduplication (
internal/cmd)initTracingProviderWithFallback(...)shutdownTracingProviderWithTimeout(...)internal/cmd/root.gointernal/cmd/proxy.goHTTP server creation deduplication (
internal/server)newHTTPServer(addr, handler)ininternal/server/http_server.go.&http.Server{Addr, Handler}construction in:internal/server/transport.gointernal/server/routed.goFocused test additions
internal/cmd/tracing_helpers_test.goto verify tracing flag defaults and binding behavior.internal/server/http_server_test.goto verify shared HTTP server helper wiring.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build3127029850/b510/launcher.test /tmp/go-build3127029850/b510/launcher.test -test.testlogfile=/tmp/go-build3127029850/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true rotocol/go-sdk@v1.5.0/auth/auth.go rotocol/go-sdk@v1.5.0/auth/authorization_code.go x_amd64/vet -p github.com/tetra-atomic -lang=go1.24 x_amd64/vet 4815�� g_.a -I x_amd64/vet --gdwarf-5 go-sdk/mcp -o x_amd64/vet(dns block)/tmp/go-build3648569470/b514/launcher.test /tmp/go-build3648569470/b514/launcher.test -test.testlogfile=/tmp/go-build3648569470/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build3127029850/b492/config.test /tmp/go-build3127029850/b492/config.test -test.testlogfile=/tmp/go-build3127029850/b492/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true mt-go@v0.1.8/format.go mt-go@v0.1.8/parse.go x_amd64/vet --gdwarf-5 backoff -o x_amd64/vet 4815�� g_.a pkg/mod/go.opent-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet(dns block)/tmp/go-build3648569470/b496/config.test /tmp/go-build3648569470/b496/config.test -test.testlogfile=/tmp/go-build3648569470/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s(dns block)nonexistent.local/tmp/go-build3127029850/b510/launcher.test /tmp/go-build3127029850/b510/launcher.test -test.testlogfile=/tmp/go-build3127029850/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true rotocol/go-sdk@v1.5.0/auth/auth.go rotocol/go-sdk@v1.5.0/auth/authorization_code.go x_amd64/vet -p github.com/tetra-atomic -lang=go1.24 x_amd64/vet 4815�� g_.a -I x_amd64/vet --gdwarf-5 go-sdk/mcp -o x_amd64/vet(dns block)/tmp/go-build3648569470/b514/launcher.test /tmp/go-build3648569470/b514/launcher.test -test.testlogfile=/tmp/go-build3648569470/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s(dns block)slow.example.com/tmp/go-build3127029850/b510/launcher.test /tmp/go-build3127029850/b510/launcher.test -test.testlogfile=/tmp/go-build3127029850/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true rotocol/go-sdk@v1.5.0/auth/auth.go rotocol/go-sdk@v1.5.0/auth/authorization_code.go x_amd64/vet -p github.com/tetra-atomic -lang=go1.24 x_amd64/vet 4815�� g_.a -I x_amd64/vet --gdwarf-5 go-sdk/mcp -o x_amd64/vet(dns block)/tmp/go-build3648569470/b514/launcher.test /tmp/go-build3648569470/b514/launcher.test -test.testlogfile=/tmp/go-build3648569470/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s(dns block)this-host-does-not-exist-12345.com/tmp/go-build3127029850/b519/mcp.test /tmp/go-build3127029850/b519/mcp.test -test.testlogfile=/tmp/go-build3127029850/b519/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true .cfg -I x_amd64/vet -I . -imultiarch x_amd64/vet -W .cfg om/segmentio/enc-ifaceassert x_amd64/vet . g/grpc/internal/--version --64 x_amd64/vet(dns block)/tmp/go-build3648569470/b523/mcp.test /tmp/go-build3648569470/b523/mcp.test -test.testlogfile=/tmp/go-build3648569470/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -uns�� -unreachable=false /tmp/go-build3127029850/b025/vet.cfg ache/go/1.25.8/x64/pkg/tool/linux_amd64/compile rnal/filetype/bubash ache/go/1.25.8/x/usr/bin/runc x_amd64/compile ache/go/1.25.8/x64/pkg/tool/linux_amd64/compile -uns�� 7029850/b512/_pkg_.a /tmp/go-build3127029850/b122/vet.cfg 7029850/b512=> .go b/gh-aw-mcpg/int/usr/bin/runc x_amd64/compile /opt/hostedtoolcache/go/1.25.8/xorigin(dns block)If you need me to access, download, or install something from one of these locations, you can either: