Skip to content

feat: Add error logging for HttpClientSseClientTransport #361

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

tiaoc
Copy link

@tiaoc tiaoc commented Jul 1, 2025

Modified the HTTP request error handling to now capture and log the response body content (via response.body()) for non-success status codes (non-200/201/202/206), rather than only logging the status code.

Motivation and Context

When a failed status code is received, ​​the response lacks a body​​, forcing developers to manually reproduce the issue.

How Has This Been Tested?

Constructed an improperly configured HTTP client request builder and verified the log message output via unit testing.​

Breaking Changes

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

Using ofString() to read the response body aids in troubleshooting server errors, but ​​incurs serialization overhead​​ (even for trivial bodies like "Accepted"). If maintainers reject this change, the method requires a ​​documentation comment instructing developers to manually inspect the response body​​ during debugging.

tzolov added a commit that referenced this pull request Jul 13, 2025
- Include response body in error messages for better debugging
- Change sendHttpPost return type from HttpResponse<Void> to HttpResponse<String>
- Use BodyHandlers.ofString() instead of discarding response body
- Add testErrorOnBogusMessage test to verify error handling for invalid JSON-RPC messages
- Test validates that malformed messages return 400 status with descriptive error message

Replace #361

Signed-off-by: Christian Tzolov <[email protected]>

Co-authored-by: jkma <[email protected]>
@tzolov
Copy link
Contributor

tzolov commented Jul 13, 2025

@tiaoc thanks you for the improvement.

you are are still a co-author

@tzolov
Copy link
Contributor

tzolov commented Jul 13, 2025

Replaced by #391

@tzolov tzolov closed this Jul 13, 2025
tzolov added a commit that referenced this pull request Jul 13, 2025
- Include response body in error messages for better debugging
- Change sendHttpPost return type from HttpResponse<Void> to HttpResponse<String>
- Use BodyHandlers.ofString() instead of discarding response body
- Add testErrorOnBogusMessage test to verify error handling for invalid JSON-RPC messages
- Test validates that malformed messages return 400 status with descriptive error message

Replace #361

Signed-off-by: Christian Tzolov <[email protected]>

Co-authored-by: jkma <[email protected]>
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