mcp: Add example of using Middleware for logging purposes #58
Conversation
rwjblue-glean
force-pushed
the
rwjblue/logging-middleware
branch
from
22d44b4 to
d87030f
Compare
| // Note: The actual HTTP session ID (from Mcp-Session-Id header) is not | ||
| // accessible from middleware with the current SDK design. This would require | ||
| // SDK changes to pass the session ID through context. For now, we use the | ||
| // session pointer as a unique identifier within this process. | ||
| sessionID := fmt.Sprintf("%p", session) |
There was a problem hiding this comment.
I struggled to find a way to properly gain access to the Mcp-Session-Id, which seems kinda important. I would love to update the example to fix that if I'm just missing something super obvious.
I kinda expected to be able to do something like session.SessionId() or somesuch, though that obviously doesn't work (also, not all sessions are even going to have a session id conceptually -- e.g. stdio and in memory transports have no logical session id).
As it stands now, this "works" but really doesn't help folks that want to implement a Streamable HTTP server and have easy correlation between incoming request logs (e.g. that might have the Mcp-Session-Id header) and the middleware logging output.
There was a problem hiding this comment.
We clearly need to fix this. I'll add something.
There was a problem hiding this comment.
#67 has landed. Please try again.
There was a problem hiding this comment.
Awesome, thank you!
There was a problem hiding this comment.
This should be a Go documentation example, not a separate program. See mcp/example_progress_test.go.
You won't need a lot of test infrastructure in that case. I would disable time output, and use the "// Output:" feature of examples for some very basic testing.
src/cmd/go/alldocs.go
If that doesn't work out for some reason, let me know and I'll reconsider.
|
|
||
| var httpAddr = flag.String("http", "", "if set, use streamable HTTP at this address, instead of stdin/stdout") | ||
|
|
||
| // LoggingMiddleware provides comprehensive MCP-level logging |
There was a problem hiding this comment.
Also, rephrase to avoid "marketing" adjectives like "comprehensive". Keep it neutral. And clarify that this is server-side logging, to distinguish it from the MCP logging feature, which goes back to the client.
| // Note: The actual HTTP session ID (from Mcp-Session-Id header) is not | ||
| // accessible from middleware with the current SDK design. This would require | ||
| // SDK changes to pass the session ID through context. For now, we use the | ||
| // session pointer as a unique identifier within this process. | ||
| sessionID := fmt.Sprintf("%p", session) |
There was a problem hiding this comment.
We clearly need to fix this. I'll add something.
| "method", method, | ||
| "session_id", sessionID, | ||
| "duration_ms", duration.Milliseconds(), | ||
| "error", err.Error(), |
There was a problem hiding this comment.
Use "err". We didn't make this an exported constant in log/slog, but we want it to be the convention.
There was a problem hiding this comment.
Also, I think you can pass in err and its Error method will get called. Confirm that.
|
Thank you @jba for taking a look! I will work on updating this evening. |
Motivation and Context
I needed to implement some logging for our experimental server that we are building out using the SDK, and figured I could propose an example that might make it easier for folks in the future.
For #33
How Has This Been Tested?
Implemented a small number of tests to ensure that the example continues to function over time.
Breaking Changes
N/A
Types of changes
Checklist
Additional context