Skip to content

refactor(mcp):used logger to log instead of print and updated unittes… #4324

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.

Sign up for GitHub

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
DineshThumma9 wants to merge 4 commits into
base: main
Choose a base branch
from
+10 −12
Open

refactor(mcp):used logger to log instead of print and updated unittes… #4324

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
01be269
refactor(mcp):used logger to log instead of print and updated unittes…
DineshThumma9 Jan 30, 2026
3a19d07
refactored with suggested changes
DineshThumma9 Jan 30, 2026
ed9c451
Merge branch 'main' into logger_in_mcp_session_clean
DineshThumma9 Jan 30, 2026
ceee2b3
Merge branch 'main' into logger_in_mcp_session_clean
wuliang229 Jan 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/google/adk/tools/mcp_tool/mcp_session_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,10 +434,9 @@ async def close(self):
await exit_stack.aclose()
except Exception as e:
# Log the error but don't re-raise to avoid blocking shutdown
print(
'Warning: Error during MCP session cleanup for'
f' {session_key}: {e}',
file=self._errlog,
logger.warning(
f'Warning: Error during MCP session cleanup for:{session_key}',
exc_info=True,
)
Comment on lines 437 to 440
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 30, 2026

Choose a reason for hiding this comment

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

medium

The log message can be improved for clarity and to follow standard logging practices. The logger.warning call already indicates the severity level, so including 'Warning:' in the message string is redundant. Also, a missing space after the colon in the f-string affects readability.

Suggested change
logger.warning(
f'Warning: Error during MCP session cleanup for:{session_key}',
exc_info=True,
)
logger.warning(
f'Error during MCP session cleanup for {session_key}',
exc_info=True,
)

finally:
del self._sessions[session_key]
Expand Down
14 changes: 6 additions & 8 deletions tests/unittests/tools/mcp_tool/test_mcp_session_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,8 @@ async def test_close_success(self):
assert len(manager._sessions) == 0

@pytest.mark.asyncio
async def test_close_with_errors(self):
@patch("google.adk.tools.mcp_tool.mcp_session_manager.logger")
async def test_close_with_errors(self, mock_logger):
"""Test cleanup when some sessions fail to close."""
manager = MCPSessionManager(self.mock_stdio_connection_params)

Expand All @@ -403,20 +404,17 @@ async def test_close_with_errors(self):
manager._sessions["session1"] = (session1, exit_stack1)
manager._sessions["session2"] = (session2, exit_stack2)

custom_errlog = StringIO()
manager._errlog = custom_errlog

# Should not raise exception
await manager.close()

# Good session should still be closed
exit_stack2.aclose.assert_called_once()
assert len(manager._sessions) == 0

# Error should be logged
error_output = custom_errlog.getvalue()
assert "Warning: Error during MCP session cleanup" in error_output
assert "Close error 1" in error_output
# Error should be logged via logger.warning
mock_logger.warning.assert_called()
warning_call = str(mock_logger.warning.call_args)
assert "Warning: Error during MCP session cleanup" in warning_call
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 30, 2026

Choose a reason for hiding this comment

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

medium

The test assertion for the log message can be made more robust. Instead of converting call_args to a string, it's better to inspect the arguments passed to mock_logger.warning directly. This makes the test less fragile and more explicit about what's being tested. Using assert_called_once() is also more precise than assert_called() when you expect a single call. This suggestion also aligns with the proposed change to the log message format.

Suggested change
mock_logger.warning.assert_called()
warning_call = str(mock_logger.warning.call_args)
assert "Warning: Error during MCP session cleanup" in warning_call
mock_logger.warning.assert_called_once()
args, kwargs = mock_logger.warning.call_args
assert 'Error during MCP session cleanup for session1' in args[0]
assert kwargs.get('exc_info')


@pytest.mark.asyncio
@patch("google.adk.tools.mcp_tool.mcp_session_manager.stdio_client")
Expand Down