Skip to content

Commit df79a19

Browse files
authored
refactor: deduplicate reconnect telemetry and SSE deprecation logging in connection.go (#3660)
Three duplicate logging patterns in `internal/mcp/connection.go` created maintainability risk: reconnect lifecycle logs were copy-pasted verbatim across two sibling functions, and SSE deprecation connected with a 5-line near-identical warning burst. ## Changes - **Reconnect telemetry helpers** — extracted `logReconnectStart()` and `logReconnectResult(err error)` methods; both `reconnectPlainJSON()` and `reconnectSDKTransport()` now delegate to them instead of duplicating three `logger.Log*` call sites each: ```go func (c *Connection) logReconnectStart() { logger.LogWarn("backend", "MCP session expired for %s, attempting to reconnect...", c.serverID) } func (c *Connection) logReconnectResult(err error) { if err != nil { logger.LogError("backend", "Session reconnect failed for %s: %v", c.serverID, err) } else { logger.LogInfo("backend", "Session successfully reconnected for %s", c.serverID) } } ``` - **SSE deprecation warning** — collapsed 5 overlapping `LogWarn` calls (all saying "SSE is deprecated, please migrate") into one `LogWarn` with the URL + spec version, plus one `LogInfo` for the connection confirmation. Issue #3633 (redundant `log.Printf` / `logger.LogInfo` pairs) was already resolved prior to this PR. > [!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-build3844589215/b514/launcher.test /tmp/go-build3844589215/b514/launcher.test -test.testlogfile=/tmp/go-build3844589215/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W .cfg elemetry.io/otel-ifaceassert x_amd64/vet ache/go/1.25.8/x/usr/libexec/docker/cli-plugins/docker-compose --gdwarf2 --64 x_amd64/vet -I .cfg om/yosida95/uritemplate/v3@v3.0.2/equals.go x_amd64/vet --gdwarf-5 g/protobuf/inter--version -o x_amd64/vet` (dns block) > - `https://api.github.com/graphql` > - Triggering command: `/usr/bin/gh gh issue close 3634 --comment Fixed in PR #3660: Consolidated the 5-line SSE deprecation warning burst into 2 concise log calls ��� one `LogWarn` containing the URL, spec version, and migration ask, and one `LogInfo` confirming the connection. This eliminates the near-duplicate messag ock - Extract logReconnectStart() and logReconnectResult(err) helpers to deduplicate the three identical logger.Log* call sites shared by reconnectPlainJSON() and reconne` (http block) > - Triggering command: `/usr/bin/gh gh issue close 3632 --comment Fixed in PR #3660: Extracted `logReconnectStart()` and `logReconnectResult(err)` helper methods into `connection.go`. Both `reconnectPlainJSON()` and `reconnectSDKTransport()` now delegate their three shared `logger.Log*` call sites to these helpers, elim iVcYPCLQvuKP by/fbd8401427f0fc669ca1ec4ad2768json D8KssBkgN 059210538/b288///opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/go/tools/autobuild.sh` (http block) > - Triggering command: `/usr/bin/gh gh issue comment 3634 --body Fixed in PR #3660: Consolidated the 5-line SSE deprecation warning burst into 2 concise log calls ��� one `LogWarn` containing the URL, spec version, and migration ask, and one `LogInfo` confirming the connection. --repo github/gh-aw-mcpg ntime.v2.task/mogit om/tetratelabs/w-c x_amd64/vet egration/start_glog 4589�� .go y /bin/java ntime.v2.task/mogit rg/x/net@v0.52.0add x_amd64/vet /bin/java` (http block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build3844589215/b496/config.test /tmp/go-build3844589215/b496/config.test -test.testlogfile=/tmp/go-build3844589215/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3844589215/b396/vet.cfg @v1.1.3/cpu/cpu.-errorsas om/tetratelabs/w-ifaceassert x_amd64/vet --gdwarf-5 nal/encoding/def-atomic -o x_amd64/vet -I hB8eipdrZ -I x_amd64/vet --gdwarf-5 9210538/b239/ -o x_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build3844589215/b514/launcher.test /tmp/go-build3844589215/b514/launcher.test -test.testlogfile=/tmp/go-build3844589215/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W .cfg elemetry.io/otel-ifaceassert x_amd64/vet ache/go/1.25.8/x/usr/libexec/docker/cli-plugins/docker-compose --gdwarf2 --64 x_amd64/vet -I .cfg om/yosida95/uritemplate/v3@v3.0.2/equals.go x_amd64/vet --gdwarf-5 g/protobuf/inter--version -o x_amd64/vet` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build3844589215/b514/launcher.test /tmp/go-build3844589215/b514/launcher.test -test.testlogfile=/tmp/go-build3844589215/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W .cfg elemetry.io/otel-ifaceassert x_amd64/vet ache/go/1.25.8/x/usr/libexec/docker/cli-plugins/docker-compose --gdwarf2 --64 x_amd64/vet -I .cfg om/yosida95/uritemplate/v3@v3.0.2/equals.go x_amd64/vet --gdwarf-5 g/protobuf/inter--version -o x_amd64/vet` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build3844589215/b523/mcp.test /tmp/go-build3844589215/b523/mcp.test -test.testlogfile=/tmp/go-build3844589215/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3844589215/b517/_pkg_.a 64/src/os/signal/sig.s -I x_amd64/vet --gdwarf-5 g/grpc/credentiainspect -o x_amd64/vet -W /auth/apikey.go /auth/header.go x_amd64/vet . --gdwarf2 --64 x_amd64/vet` (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 911405c + 129e87f commit df79a19

File tree

1 file changed

+21
-11
lines changed

1 file changed

+21
-11
lines changed

internal/mcp/connection.go

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -243,11 +243,8 @@ func NewHTTPConnection(ctx context.Context, serverID, url string, headers map[st
243243
logConn.Printf("Attempting SSE transport for %s", url)
244244
conn, err = trySSETransport(ctx, cancel, serverID, url, headers, headerClient, keepAlive)
245245
if err == nil {
246-
logger.LogWarn("backend", "⚠️ MCP over SSE has been deprecated. Connected using SSE transport for url=%s. Please migrate to streamable HTTP transport (2025-03-26 spec).", url)
247-
logger.LogWarn("backend", "⚠️ WARNING: MCP over SSE (2024-11-05 spec) has been DEPRECATED")
248-
logger.LogWarn("backend", "⚠️ The server at %s is using the deprecated SSE transport", url)
249-
logger.LogWarn("backend", "⚠️ Please migrate to streamable HTTP transport (2025-03-26 spec)")
250-
logger.LogWarn("backend", "Configured HTTP MCP server with SSE transport: %s", url)
246+
logger.LogWarn("backend", "⚠️ MCP over SSE (2024-11-05 spec) is DEPRECATED for url=%s. Please migrate to streamable HTTP transport (2025-03-26 spec).", url)
247+
logger.LogInfo("backend", "Configured HTTP MCP server with SSE transport: %s", url)
251248
return conn, nil
252249
}
253250
logConn.Printf("SSE transport failed: %v", err)
@@ -296,6 +293,21 @@ func (c *Connection) ServerInfo() (name, version string) {
296293
return initResult.ServerInfo.Name, initResult.ServerInfo.Version
297294
}
298295

296+
// logReconnectStart emits the structured log warning that is common to all reconnect paths.
297+
func (c *Connection) logReconnectStart() {
298+
logger.LogWarn("backend", "MCP session expired for %s, attempting to reconnect...", c.serverID)
299+
}
300+
301+
// logReconnectResult emits the structured log entry that signals whether the reconnect
302+
// succeeded or failed. It is the common success/failure telemetry shared by all reconnect paths.
303+
func (c *Connection) logReconnectResult(err error) {
304+
if err != nil {
305+
logger.LogError("backend", "Session reconnect failed for %s: %v", c.serverID, err)
306+
} else {
307+
logger.LogInfo("backend", "Session successfully reconnected for %s", c.serverID)
308+
}
309+
}
310+
299311
// reconnectPlainJSON re-initialises the plain JSON-RPC session with the HTTP backend.
300312
// It is safe for concurrent callers: only one reconnect runs at a time, and the updated
301313
// session ID is available to all callers once the lock is released.
@@ -304,17 +316,16 @@ func (c *Connection) reconnectPlainJSON() error {
304316
defer c.sessionMu.Unlock()
305317

306318
logConn.Printf("Session expired, reconnecting plain JSON-RPC for serverID=%s", c.serverID)
307-
logger.LogWarn("backend", "MCP session expired for %s, attempting to reconnect...", c.serverID)
319+
c.logReconnectStart()
308320

309321
sessionID, err := c.initializeHTTPSession()
322+
c.logReconnectResult(err)
310323
if err != nil {
311-
logger.LogError("backend", "Session reconnect failed for %s: %v", c.serverID, err)
312324
return fmt.Errorf("session reconnect failed: %w", err)
313325
}
314326

315327
c.httpSessionID = sessionID
316328
logConn.Printf("Reconnected plain JSON-RPC session for serverID=%s, new sessionID=%s", c.serverID, sessionID)
317-
logger.LogInfo("backend", "Session successfully reconnected for %s", c.serverID)
318329
return nil
319330
}
320331

@@ -325,7 +336,7 @@ func (c *Connection) reconnectSDKTransport() error {
325336
defer c.sessionMu.Unlock()
326337

327338
logConn.Printf("Session expired, reconnecting SDK transport for serverID=%s, type=%s", c.serverID, c.httpTransportType)
328-
logger.LogWarn("backend", "MCP session expired for %s, attempting to reconnect...", c.serverID)
339+
c.logReconnectStart()
329340

330341
// Close the existing session gracefully (ignore error – it's already dead).
331342
if c.session != nil {
@@ -359,16 +370,15 @@ func (c *Connection) reconnectSDKTransport() error {
359370
defer cancel()
360371

361372
session, err := client.Connect(connectCtx, transport, nil)
373+
c.logReconnectResult(err)
362374
if err != nil {
363-
logger.LogError("backend", "Session reconnect failed for %s: %v", c.serverID, err)
364375
return fmt.Errorf("session reconnect failed: %w", err)
365376
}
366377

367378
c.client = client
368379
c.session = session
369380

370381
logConn.Printf("Reconnected SDK session for serverID=%s", c.serverID)
371-
logger.LogInfo("backend", "Session successfully reconnected for %s", c.serverID)
372382
return nil
373383
}
374384

0 commit comments

Comments
 (0)