Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rwjblue-glean
Copy link
Contributor

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@rwjblue-glean rwjblue-glean force-pushed the rwjblue/logging-middleware branch from 22d44b4 to d87030f Compare June 26, 2025 23:44
Comment on lines +36 to +40
// 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)
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #67.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you!

Copy link
Contributor

@jba jba left a 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
Copy link
Contributor

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

Copy link
Contributor

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.

Comment on lines +36 to +40
// 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)
Copy link
Contributor

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(),
Copy link
Contributor

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.

Copy link
Contributor

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.

@rwjblue-glean
Copy link
Contributor Author

Thank you @jba for taking a look! I will work on updating this evening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants