Skip to content

Commit 18117eb

Browse files
authored
Integrate project logger with MCP SDK ClientOptions and ServerOptions (#1067)
The MCP SDK v1.3.0 supports logger injection through `ClientOptions` and `ServerOptions`, but the gateway was passing empty options (`&sdk.ClientOptions{}`) or `nil`, leaving SDK operations invisible in logs. ## Changes **Client creation** (`internal/mcp/connection.go`) - `newMCPClient()` now accepts `*logger.Logger` parameter - Converts to `*slog.Logger` via `logger.NewSlogLoggerWithHandler()` - Passes to `sdk.ClientOptions.Logger` **Server creation** (`internal/server/unified.go`, `internal/server/routed.go`) - Pass logger via `sdk.ServerOptions` in unified mode - Pass logger via `sdk.ServerOptions` in routed/filtered servers **Tests** - Tests pass `nil` to disable SDK logging in test environments - Added `TestNewMCPClientWithLogger` for validation ## Example Before: ```go sdk.NewClient(&sdk.Implementation{...}, &sdk.ClientOptions{}) sdk.NewServer(&sdk.Implementation{...}, nil) ``` After: ```go sdk.NewClient(&sdk.Implementation{...}, &sdk.ClientOptions{ Logger: logger.NewSlogLoggerWithHandler(log), }) sdk.NewServer(&sdk.Implementation{...}, &sdk.ServerOptions{ Logger: logger.NewSlogLoggerWithHandler(logUnified), }) ``` SDK internal operations now flow through the project's debug logging system (`DEBUG=*` shows SDK activity). > [!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-build1436996487/b275/launcher.test /tmp/go-build1436996487/b275/launcher.test -test.testlogfile=/tmp/go-build1436996487/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true o s.go 64/pkg/tool/linu_x004.c credential.helpe/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/vet` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1436996487/b260/config.test /tmp/go-build1436996487/b260/config.test -test.testlogfile=/tmp/go-build1436996487/b260/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go ac/hmac.go x_amd64/asm --depth 2 REDACTED x_amd64/asm conf�� g_.a HEHYCbGHr 64/pkg/tool/linux_amd64/compile user.name` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build1436996487/b275/launcher.test /tmp/go-build1436996487/b275/launcher.test -test.testlogfile=/tmp/go-build1436996487/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true o s.go 64/pkg/tool/linu_x004.c credential.helpe/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/vet` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build1436996487/b275/launcher.test /tmp/go-build1436996487/b275/launcher.test -test.testlogfile=/tmp/go-build1436996487/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true o s.go 64/pkg/tool/linu_x004.c credential.helpe/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/vet` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1436996487/b284/mcp.test /tmp/go-build1436996487/b284/mcp.test -test.testlogfile=/tmp/go-build1436996487/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -c=4 -nolocalimports -importcfg /tmp/go-build180286792/b223/importcfg -pack /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/middleware/jqschema.go 64/pkg/include 64/s�� 64/src/runtime/cgo --global ache/go/1.25.7/x64/pkg/tool/linu--64 pull.rebase` (dns block) > - Triggering command: `/tmp/go-build2197604316/b284/mcp.test /tmp/go-build2197604316/b284/mcp.test -test.testlogfile=/tmp/go-build2197604316/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.7/x64/src/runtime/c-p v5H2/gdzdyx1nTJ_thj3Av5H2 8085717/b189/` (dns block) > - Triggering command: `/tmp/go-build1032557839/b284/mcp.test /tmp/go-build1032557839/b284/mcp.test -test.testlogfile=/tmp/go-build1032557839/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true s mn4I/VW84qF8_Uqs9FMGXmn4I x_amd64/compile` (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> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> ---- *This section details on the original issue you should resolve* <issue_title>[go-fan] Go Module Review: modelcontextprotocol/go-sdk</issue_title> <issue_description># 🐹 Go Fan Report: modelcontextprotocol/go-sdk ## Module Overview The **modelcontextprotocol/go-sdk** is the official Go SDK for Model Context Protocol servers and clients, maintained in collaboration with Google. This SDK provides comprehensive support for building MCP servers and clients in Go, including HTTP/SSE transport, stdio transport, tool/resource/prompt registration, and session management. **Repository**: https://github.com/modelcontextprotocol/go-sdk **Version Used**: v1.3.0 ✅ (Latest) **Last Updated**: 2026-02-18 (Today!) **Stars**: 3,875 ⭐ ## Current Usage in gh-aw-mcpg ### Files & Import Statistics - **Files**: 20 files (5 production + 15 test files) - **Import Count**: 20 direct imports - **Key APIs Used**: - Server APIs: `NewServer()`, `AddTool()`, `NewStreamableHTTPHandler()`, `StreamableHTTPOptions` - Client APIs: `NewClient()`, `Connect()`, `ListTools()`, `CallTool()` - Transport APIs: `NewInMemoryTransports()`, StreamableHTTP with session support ### Production Files 1. `internal/server/transport.go` - StreamableHTTP handler creation 2. `internal/server/unified.go` - Unified mode server implementation 3. `internal/server/routed.go` - Routed mode server implementation 4. `internal/mcp/connection.go` - Client connections to backend MCP servers 5. `internal/testutil/mcptest/` - Test infrastructure (3 files) ### Current Patterns **Server Creation:** ```go sdk.NewServer(&sdk.Implementation{ Name: "awmg-unified", Version: "1.0.0", }, nil) // ⚠️ No server options used ``` **Client Creation:** ```go sdk.NewClient(&sdk.Implementation{ Name: "awmg", Version: version.Get(), }, &sdk.ClientOptions{}) // ⚠️ Empty options ``` **StreamableHTTP (Good!):** ```go &sdk.StreamableHTTPOptions{ Stateless: false, Logger: logger.NewSlogLoggerWithHandler(logTransport), SessionTimeout: 30 * time.Minute, } ``` ## Research Findings ### Recent Updates (v1.3.0 - Released 2026-02-09) The project is using the **latest version** of the SDK! v1.3.0 includes significant improvements: 1. **✨ Schema Caching** - Dramatically improves performance in stateless server scenarios 2. **🔧 Logger in ClientOptions** - Logging moved from deprecated location to `ClientOptions` 3. **🛡️ GetError/SetError Export** - Better structured error handling 4. **🐛 Case-Insensitive HTTP Headers** - Fixed SSE Content-Type checking bug 5. **⚡ Race Condition Fix** - Fixed logging race condition 6. **📋 HTTP 405 Compliance** - Added Allow header per RFC 9110 ### Best Practices from SDK Maintainers 1. **Logger Integration**: Pass logger through `ClientOptions` and `ServerOptions` for complete visibility 2. **Error Handling**: Use SDK's `GetError/SetError` methods for structured error context 3. **Session Management**: Leverage `StreamableHTTP` with timeouts (✅ already doing this!) 4. **Schema Caching**: Automatic in v1.3.0, provides significant performance gains ### SDK Features We're NOT Using 1. **Resources API** - Only test usage, no production resources exposed 2. **Prompts API** - Zero usage (could provide helpful templates) 3. **Completion API** - Zero usage 4. **Sampling API** - Zero usage (LLM integration) 5. **Extensions (SEP-2133)** - New capability for custom protocol extensions 6. **Advanced Logging** - Logger only in transport, not in client/server options 7. **Structured Error Types** - Custom HTTP errors instead of SDK types ## Improvement Opportunities ### 🏃 Quick Wins #### 1. Add Logger to ClientOptions ⭐ HIGH PRIORITY - **Current**: `&sdk.ClientOptions{}` (empty) - **Fix**: Pass project logger to SDK - **Benefit**: Unified logging across gateway and SDK operations - **Effort**: 1-2 hours - **Location**: `internal/mcp/connection.go:165` - **Code**: ```go &sdk.ClientOptions{ Logger: logger.NewSlogLoggerWithHandler(logClient), } ``` #### 2. Add Logger to ServerOptions ⭐ HIGH PRIORITY - **Current**: `sdk.NewServer(..., nil)` (no options) - **Fix**: Pass logger via server options - **Benefit**: Better visibility into SDK server operations - **Effort**: 1-2 hours - **Locations**: `internal/server/unified.go:136`, `internal/server/routed.go:187` #### 3. Validate Schema Caching Performance - **Current**: Automatic in v1.3.0 (good!) - **Improvement**: Add metrics to quantify performance gains - **Benefit**: Document actual performance improvements - **Effort**: 2-3 hours ### ✨ Feature Opportunities #### 1. Implement Resources API (Medium Complexity) **Opportunity**: Expose gateway state as MCP resources **Potential Resources**: - `/config` - Current gateway configuration - `/health` - Gateway health status - `/metrics` - Performance metrics - `/backends` - Backend server status - `/sessions` - Active session information **Benefit**: Clients can query gateway state using ... </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #1059 <!-- START COPILOT CODING AGENT TIPS --> --- 🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. [Learn more about Advanced Security.](https://gh.io/cca-advanced-security)
2 parents 1cd5306 + 34b958c commit 18117eb

4 files changed

Lines changed: 32 additions & 12 deletions

File tree

internal/mcp/connection.go

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"fmt"
99
"io"
1010
"log"
11+
"log/slog"
1112
"net/http"
1213
"os"
1314
"os/exec"
@@ -158,11 +159,18 @@ type Connection struct {
158159
}
159160

160161
// newMCPClient creates a new MCP SDK client with standard implementation details
161-
func newMCPClient() *sdk.Client {
162+
// Pass nil for logger parameter to disable SDK logging (for tests)
163+
func newMCPClient(log *logger.Logger) *sdk.Client {
164+
var slogLogger *slog.Logger
165+
if log != nil {
166+
slogLogger = logger.NewSlogLoggerWithHandler(log)
167+
}
162168
return sdk.NewClient(&sdk.Implementation{
163169
Name: "awmg",
164170
Version: version.Get(),
165-
}, &sdk.ClientOptions{})
171+
}, &sdk.ClientOptions{
172+
Logger: slogLogger,
173+
})
166174
}
167175

168176
// newHTTPConnection creates a new HTTP Connection struct with common fields
@@ -300,8 +308,8 @@ func NewConnection(ctx context.Context, serverID, command string, args []string,
300308
logConn.Printf("Creating new MCP connection: command=%s, args=%v", command, sanitize.SanitizeArgs(args))
301309
ctx, cancel := context.WithCancel(ctx)
302310

303-
// Create MCP client
304-
client := newMCPClient()
311+
// Create MCP client with logger
312+
client := newMCPClient(logConn)
305313

306314
// Expand Docker -e flags that reference environment variables
307315
// Docker's `-e VAR_NAME` expects VAR_NAME to be in the environment
@@ -506,8 +514,8 @@ func trySDKTransport(
506514
transportName string,
507515
createTransport transportConnector,
508516
) (*Connection, error) {
509-
// Create MCP client
510-
client := newMCPClient()
517+
// Create MCP client with logger
518+
client := newMCPClient(logConn)
511519

512520
// Create transport using the provided connector
513521
transport := createTransport(url, httpClient)

internal/mcp/connection_test.go

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"strings"
1212
"testing"
1313

14+
"github.com/github/gh-aw-mcpg/internal/logger"
1415
"github.com/stretchr/testify/assert"
1516
"github.com/stretchr/testify/require"
1617
)
@@ -568,10 +569,17 @@ data: {"jsonrpc":"2.0","id":` + idStr + `,"result":{"tools":[]}}
568569

569570
// TestNewMCPClient tests the newMCPClient helper function
570571
func TestNewMCPClient(t *testing.T) {
571-
client := newMCPClient()
572+
client := newMCPClient(nil)
572573
require.NotNil(t, client, "newMCPClient should return a non-nil client")
573574
}
574575

576+
// TestNewMCPClientWithLogger tests that newMCPClient accepts a logger
577+
func TestNewMCPClientWithLogger(t *testing.T) {
578+
log := logger.New("test:client")
579+
client := newMCPClient(log)
580+
require.NotNil(t, client, "newMCPClient should return a non-nil client with logger")
581+
}
582+
575583
// TestCreateJSONRPCRequest tests the createJSONRPCRequest helper function
576584
func TestCreateJSONRPCRequest(t *testing.T) {
577585
tests := []struct {
@@ -687,7 +695,7 @@ func TestNewHTTPConnection(t *testing.T) {
687695
ctx, cancel := context.WithCancel(context.Background())
688696
defer cancel()
689697

690-
client := newMCPClient()
698+
client := newMCPClient(nil)
691699
url := "http://example.com/mcp"
692700
headers := map[string]string{"Authorization": "test"}
693701
httpClient := &http.Client{}

internal/server/routed.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -183,11 +183,13 @@ func CreateHTTPServerForRoutedMode(addr string, unifiedServer *UnifiedServer, ap
183183
func createFilteredServer(unifiedServer *UnifiedServer, backendID string) *sdk.Server {
184184
logRouted.Printf("Creating filtered server: backend=%s", backendID)
185185

186-
// Create a new SDK server for this route
186+
// Create a new SDK server for this route with logger
187187
server := sdk.NewServer(&sdk.Implementation{
188188
Name: fmt.Sprintf("awmg-%s", backendID),
189189
Version: "1.0.0",
190-
}, nil)
190+
}, &sdk.ServerOptions{
191+
Logger: logger.NewSlogLoggerWithHandler(logRouted),
192+
})
191193

192194
// Get tools for this backend from the unified server
193195
tools := unifiedServer.GetToolsForBackend(backendID)

internal/server/unified.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,11 +132,13 @@ func NewUnified(ctx context.Context, cfg *config.Config) (*UnifiedServer, error)
132132
enableDIFC: cfg.EnableDIFC,
133133
}
134134

135-
// Create MCP server
135+
// Create MCP server with logger
136136
server := sdk.NewServer(&sdk.Implementation{
137137
Name: "awmg-unified",
138138
Version: "1.0.0",
139-
}, nil)
139+
}, &sdk.ServerOptions{
140+
Logger: logger.NewSlogLoggerWithHandler(logUnified),
141+
})
140142

141143
us.server = server
142144

0 commit comments

Comments
 (0)