-
Notifications
You must be signed in to change notification settings - Fork 270
mcp/server: enable server logging #501
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
Conversation
mcp/server.go
Outdated
| opts ServerOptions | ||
| impl *Implementation | ||
| opts ServerOptions | ||
| logger *slog.Logger |
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 opts.Logger?
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.
updated code to use opts.Logger
mcp/server.go
Outdated
| type ServerOptions struct { | ||
| // Optional instructions for connected clients. | ||
| Instructions string | ||
| // Logger is used for server-side logging. If nil, disable 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.
// If non-nil, log server activity.
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.
updated comment
mcp/server.go
Outdated
| }) | ||
|
|
||
| if !wasInit { | ||
| ss.server.logger.Warn("initialized before initialize") |
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.
If we are returning an error, shouldn't we log these as errors?
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.
changed from Warn to Error
mcp/streamable.go
Outdated
| type StreamableHTTPHandler struct { | ||
| getServer func(*http.Request) *Server | ||
| opts StreamableHTTPOptions | ||
| logger *slog.Logger |
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 opts.Logger
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.
updated code to use opts.Logger
|
@jba addressed all of your review comments, can you please have a look. |
|
Could you use a logging interface instead of |
|
@dmarkhas You can write a slog.Handler to wrap your logger. |
|
This is ready to merge, but I recommend we do it after v1.0.0 to avoid last-minute changes. @findleyr |
|
@findleyr @samthanawalla just sign off on the public API. |
mcp/server.go: implement server‑side logging
All tests are passing and no functionality should change with this PR
Fixes #170