-
Notifications
You must be signed in to change notification settings - Fork 46
mcp: Add example of using Middleware for logging purposes #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
mcp: Add example of using Middleware for logging purposes #58
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We clearly need to fix this. I'll add something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #67.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#67 has landed. Please try again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: end with a period
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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