Skip to content

mcp/server.go: implement server-side logging throughout codebase #306

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

KamdynS
Copy link
Contributor

@KamdynS KamdynS commented Aug 15, 2025

mcp/server.go: implement server‑side logging

  • Thread a *slog.Logger through server paths to improve debuggability
  • Introduce no‑op discard handler + ensureLogger so loggers are never nil
  • Use ensured loggers in Server, SSEHandler/SSEServerTransport; remove nil checks
  • Route jsonrpc2 internal errors to an internal logger

All tests are passing and no functionality should change with this PR

Fixes #170

@findleyr findleyr requested a review from jba August 19, 2025 13:37
@@ -50,6 +52,8 @@ type Server struct {
type ServerOptions struct {
// Optional instructions for connected clients.
Instructions string
// Logger is used for server-side logging. If nil, slog.Default() is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

nil should mean no logging.

@@ -108,9 +112,11 @@ func NewServer(impl *Implementation, opts *ServerOptions) *Server {
if opts.UnsubscribeHandler != nil && opts.SubscribeHandler == nil {
panic("UnsubscribeHandler requires SubscribeHandler")
}
l := ensureLogger(opts.Logger)
Copy link
Contributor

Choose a reason for hiding this comment

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

Inline this into L119

@@ -181,6 +198,7 @@ func (t *SSEServerTransport) Connect(context.Context) (Connection, error) {
}

func (h *SSEHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
h.ensureLogger()
Copy link
Contributor

Choose a reason for hiding this comment

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

If this only occurs in one place, inline it.

@ramongalate
Copy link

could this also extend to streamable http?
I can add it to this, if needed

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.

server-side logging
3 participants