feat: allow to set a custom logger in the SSE and STDIO clients #525
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
So it logs to a logger instead of stdout. Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
WalkthroughStructured logging support was added to the SSE and Stdio clients by introducing logger fields and associated configuration options. For the StreamableHTTP transport, the logger configuration function was renamed for clarity, with a deprecated wrapper retained for backward compatibility. New tests were added to verify error logging in SSE and Stdio transports. Minor formatting and documentation updates were also made. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Suggested labels
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
IamNator
approved these changes
Aug 3, 2025
|
@caarlos0 this looks good. Please add some tests and possibly some documentation in |
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
|
@ezynda3 added tests, docs, and also implemented with logger for STDIO 🙏🏻 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
client/sse.go(0 hunks)client/transport/sse.go(7 hunks)client/transport/sse_test.go(9 hunks)client/transport/stdio.go(7 hunks)client/transport/stdio_test.go(2 hunks)client/transport/streamable_http.go(1 hunks)client/transport/streamable_http_test.go(0 hunks)www/docs/pages/clients/transports.mdx(19 hunks)
💤 Files with no reviewable changes (2)
- client/sse.go
- client/transport/streamable_http_test.go
✅ Files skipped from review due to trivial changes (2)
- client/transport/stdio.go
- www/docs/pages/clients/transports.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- client/transport/streamable_http.go
- client/transport/sse.go
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: leavez
PR: mark3labs/mcp-go#114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
📚 Learning: the sse client implementation in the mcp-go project uses a 30-second timeout for reading sse events ...
Learnt from: leavez
PR: mark3labs/mcp-go#114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
Applied to files:
client/transport/sse_test.go
📚 Learning: in the mcptest package, prefer returning errors from helper functions rather than calling t.fatalf()...
Learnt from: octo
PR: mark3labs/mcp-go#149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Applied to files:
client/transport/sse_test.goclient/transport/stdio_test.go
📚 Learning: in the mark3labs/mcp-go project, the mcpserver.capabilities field is a struct value (servercapabilit...
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Applied to files:
client/transport/sse_test.goclient/transport/stdio_test.go
📚 Learning: in go, when accessing fields or methods of embedded structs, use the direct field name without the e...
Learnt from: sunerpy
PR: mark3labs/mcp-go#484
File: server/session.go:175-175
Timestamp: 2025-07-10T09:30:05.381Z
Learning: In Go, when accessing fields or methods of embedded structs, use the direct field name without the embedded field path (e.g., `notification.Method` instead of `notification.Notification.Method`). Using the full path triggers golangci-lint QF1008 warning and violates Go conventions for embedded fields.
Applied to files:
client/transport/sse_test.go
📚 Learning: in mcp-go project, the maintainer prefers keeping builder pattern apis simple without excessive vali...
Learnt from: davidleitw
PR: mark3labs/mcp-go#451
File: mcp/tools.go:1192-1217
Timestamp: 2025-06-26T09:38:18.629Z
Learning: In mcp-go project, the maintainer prefers keeping builder pattern APIs simple without excessive validation for edge cases. The WithOutput* functions are designed to assume correct usage rather than defensive programming, following the principle of API simplicity over comprehensive validation.
Applied to files:
client/transport/stdio_test.go
🪛 GitHub Check: lint
client/transport/stdio_test.go
[failure] 556-556:
Error return value of stdoutWriter.Write is not checked (errcheck)
🪛 GitHub Actions: golangci-lint
client/transport/stdio_test.go
[error] 556-556: golangci-lint: Error return value of stdoutWriter.Write is not checked (errcheck)
🔇 Additional comments (3)
client/transport/stdio_test.go (1)
8-8: LGTM!The new imports are appropriately added to support the new test functionality -
iofor pipe operations,stringsfor string checking, andrequirefor enhanced assertions.Also applies to: 13-13, 20-20
client/transport/sse_test.go (2)
7-7: LGTM!The import reordering follows Go conventions for better code organization, grouping standard library and third-party imports appropriately.
Also applies to: 9-10, 17-17
870-914: Excellent error logging test implementation.This test effectively verifies that JSON unmarshaling errors in SSE streams are properly logged. The test design is solid:
- Mock server sends invalid JSON to trigger the error condition
- Custom logger captures error messages for verification
- Proper cleanup and timeout handling
- Uses testify/require for enhanced assertions
The test ensures the logging improvements in the SSE transport work correctly under error conditions.
aymanbagabas
approved these changes
Aug 4, 2025
kujtimiihoxha
approved these changes
Aug 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So it logs to a logger instead of stdout.
I also deprecated
WithLoggerand added aWithHTTPLogger, so its more consistent.If you agree with these changes, I'll add tests, docs, etc.
Description
Fixes #<issue_number> (if applicable)
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation