Logging cleanup #133
Logging cleanup #133
Conversation
WalkthroughThe PR implements a structured logging infrastructure refactor across the agents-core and plugins, replacing ad-hoc logging patterns with module-level loggers and introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
agents-core/vision_agents/core/utils/logging.py (1)
102-104: Track the failing context reset issue.The TODO comment indicates
call_id_ctx.reset(token.context_token)is failing and has been commented out. This could potentially leak context across async tasks or lead to incorrect call IDs in logs.Please verify that bypassing the context var reset doesn't cause issues with concurrent calls. Do you want me to investigate potential context leakage scenarios or open an issue to track fixing this properly?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
examples/01_simple_agent_example/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
agents-core/pyproject.toml(1 hunks)agents-core/vision_agents/core/agents/agents.py(19 hunks)agents-core/vision_agents/core/events/manager.py(5 hunks)agents-core/vision_agents/core/mcp/mcp_base.py(2 hunks)agents-core/vision_agents/core/mcp/mcp_manager.py(3 hunks)agents-core/vision_agents/core/processors/base_processor.py(5 hunks)agents-core/vision_agents/core/utils/logging.py(2 hunks)plugins/cartesia/vision_agents/plugins/cartesia/tts.py(2 hunks)plugins/deepgram/vision_agents/plugins/deepgram/stt.py(3 hunks)plugins/elevenlabs/vision_agents/plugins/elevenlabs/tts.py(2 hunks)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py(8 hunks)plugins/kokoro/vision_agents/plugins/kokoro/tts.py(2 hunks)plugins/krisp/vision_agents/plugins/krisp/turn_detection.py(1 hunks)plugins/smart_turn/vision_agents/plugins/smart_turn/turn_detection.py(2 hunks)tests/test_utils.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide
Files:
plugins/kokoro/vision_agents/plugins/kokoro/tts.pyplugins/deepgram/vision_agents/plugins/deepgram/stt.pytests/test_utils.pyplugins/smart_turn/vision_agents/plugins/smart_turn/turn_detection.pyagents-core/vision_agents/core/mcp/mcp_manager.pyagents-core/vision_agents/core/events/manager.pyplugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.pyplugins/cartesia/vision_agents/plugins/cartesia/tts.pyagents-core/vision_agents/core/processors/base_processor.pyplugins/elevenlabs/vision_agents/plugins/elevenlabs/tts.pyplugins/krisp/vision_agents/plugins/krisp/turn_detection.pyagents-core/vision_agents/core/mcp/mcp_base.pyagents-core/vision_agents/core/utils/logging.pyagents-core/vision_agents/core/agents/agents.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
tests/**/*.py: Never use mocking utilities (e.g., unittest.mock, pytest-mock) in test files
Write tests using pytest (avoid unittest.TestCase or other frameworks)
Mark integration tests with @pytest.mark.integration
Do not use @pytest.mark.asyncio; async support is automatic
Files:
tests/test_utils.py
🧠 Learnings (1)
📚 Learning: 2025-10-13T22:00:34.300Z
Learnt from: dangusev
PR: GetStream/Vision-Agents#98
File: plugins/deepgram/vision_agents/plugins/deepgram/stt.py:135-150
Timestamp: 2025-10-13T22:00:34.300Z
Learning: In the Deepgram STT plugin (plugins/deepgram/vision_agents/plugins/deepgram/stt.py), the `started()` method is designed to wait for the connection attempt to complete, not to guarantee a successful connection. It's acceptable for the connection attempt to fail, and downstream code handles the case where `self.dg_connection` is `None`. The `_connected_once` event is set in the `finally` block intentionally to signal attempt completion.
Applied to files:
plugins/deepgram/vision_agents/plugins/deepgram/stt.py
🧬 Code graph analysis (4)
tests/test_utils.py (1)
agents-core/vision_agents/core/utils/logging.py (1)
configure_default_logging(20-47)
agents-core/vision_agents/core/mcp/mcp_manager.py (1)
agents-core/vision_agents/core/mcp/mcp_base.py (1)
MCPBaseServer(10-189)
agents-core/vision_agents/core/mcp/mcp_base.py (1)
agents-core/vision_agents/core/mcp/mcp_manager.py (1)
call_tool(93-111)
agents-core/vision_agents/core/agents/agents.py (8)
agents-core/vision_agents/core/utils/logging.py (4)
CallContextToken(51-55)clear_call_context(97-104)set_call_context(84-94)configure_default_logging(20-47)agents-core/vision_agents/core/llm/realtime.py (2)
Realtime(23-201)_watch_video_track(68-70)plugins/gemini/vision_agents/plugins/gemini/gemini_realtime.py (2)
Realtime(52-676)_watch_video_track(385-424)plugins/openai/vision_agents/plugins/openai/openai_realtime.py (2)
Realtime(37-460)_watch_video_track(263-267)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
add_track_subscriber(313-316)agents-core/vision_agents/core/edge/edge_transport.py (1)
add_track_subscriber(58-61)agents-core/vision_agents/core/utils/video_utils.py (1)
ensure_even_dimensions(6-26)agents-core/vision_agents/core/utils/video_forwarder.py (1)
stop(41-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
🔇 Additional comments (23)
plugins/elevenlabs/vision_agents/plugins/elevenlabs/tts.py (1)
9-9: LGTM! Clean module-level logger adoption.The shift from
logging.infoto a module-levellogger.infoimproves logging consistency and enables centralized configuration.Also applies to: 74-74
plugins/kokoro/vision_agents/plugins/kokoro/tts.py (1)
18-18: LGTM! Consistent logging pattern.Module-level logger adoption matches the pattern across other plugins.
Also applies to: 71-71
plugins/cartesia/vision_agents/plugins/cartesia/tts.py (1)
17-17: LGTM! Logging refactor aligns with PR objectives.The module-level logger pattern is consistently applied across TTS plugins.
Also applies to: 94-94
plugins/krisp/vision_agents/plugins/krisp/turn_detection.py (1)
51-53: LGTM! Improved logger initialization.Switching from a hardcoded
"KrispTurnDetection"to__name__enables automatic module-name tracking and aligns with the centralized logging pattern.agents-core/vision_agents/core/events/manager.py (4)
356-368: LGTM! Reduced log verbosity for handler registration.Downgrading handler registration messages to debug-level reduces noise while preserving diagnostics for troubleshooting.
403-403: LGTM! Protobuf event logging adjusted appropriately.Debug-level is suitable for protobuf events that aren't registered, especially when
ignore_unknown_eventsis enabled.
413-413: LGTM! Consistent debug-level for unregistered events.This message now consistently uses debug-level for events that aren't registered.
510-512: LGTM! Cancellation handling verbosity reduced.Debug-level is appropriate for internal task cancellation details during shutdown.
plugins/deepgram/vision_agents/plugins/deepgram/stt.py (2)
83-83: LGTM! Initialization verbosity reduced.Debug-level is appropriate for routine initialization and connection setup messages.
Also applies to: 136-136
251-251: Verify connection close is routine, not an error condition.The change from
warningtodebugassumes connection close is a normal shutdown event. Ensure this doesn't mask unexpected disconnections that warrant user attention.Based on learnings, the connection flow is designed to handle failures gracefully. The debug level seems appropriate if this is triggered during normal shutdown. Consider whether unexpected mid-session disconnections should remain at warning level.
plugins/smart_turn/vision_agents/plugins/smart_turn/turn_detection.py (2)
65-68: LGTM! Logger initialization improved.Using
__name__instead of"SmartTurnDetection"provides automatic module-name tracking and consistency with the broader logging refactor.
100-100: LGTM! Cleaner log formatting.Single-line warning call is more concise while preserving the message content.
agents-core/pyproject.toml (1)
29-29: Colorlog version and security check verified.The latest version of colorlog is 6.10.1, which matches the specified requirement, and no security vulnerabilities were found. The dependency is safe to use as specified.
agents-core/vision_agents/core/processors/base_processor.py (1)
22-23: LGTM! Logging modernization applied correctly.The module-level logger is properly initialized and consistently replaces direct
loggingcalls throughout the file.agents-core/vision_agents/core/mcp/mcp_manager.py (1)
11-28: LGTM! Type annotation expanded for logger compatibility.The logger type now accepts
logging.LoggerAdapterin addition tologging.Logger, enabling the MCPManager to work with the new_AgentLoggerAdapterintroduced in agents.py.agents-core/vision_agents/core/utils/logging.py (1)
20-48: LGTM! Well-designed default logging configuration.The function respects existing configuration by checking for handlers and levels before applying defaults, which prevents overriding user-specified settings.
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
33-34: LGTM! Module-level logger correctly introduced.The logger is consistently used throughout the file, replacing previous
self.loggerusage.tests/test_utils.py (2)
536-556: LGTM! Well-designed fixture for logger isolation.The factory pattern with cleanup ensures each test has an isolated logger environment and proper restoration of state.
558-594: LGTM! Comprehensive test coverage for logging configuration.The tests verify the key behaviors:
- Default configuration is applied
- Existing handlers are preserved
- Pre-set levels are not overridden
agents-core/vision_agents/core/agents/agents.py (3)
68-77: LGTM! Well-implemented logger adapter for agent context.The adapter cleanly prepends the agent ID to all log messages, making it easy to distinguish logs from multiple agents running concurrently.
127-131: LGTM! Sensible default logging configuration.The
log_levelparameter defaults toINFOand allows users to opt out by passingNone, providing good flexibility.
176-176: LGTM! Appropriate log level reduction for plugin registration.Moving plugin registration to debug level reduces noise during normal operation while keeping the information available for troubleshooting.
agents-core/vision_agents/core/mcp/mcp_base.py (1)
20-20: LGTM! Logger updated to use module-level pattern.The logger now uses
__name__which aligns with the logging modernization across the codebase.
- Replaced "logging.info" usages with separate loggers - Lowered some info messages to debug - Replaced prints with logging - Added emojis where they're already used
- Added a way to set the SDK log level at the Agent class - Set the default formatter and added level-based coloring - If the logs are already configured, they remain intact - Moved logging_utils.py to utils/logging.py
dangusev
force-pushed
the
chore/logging-cleanup
branch
from
f1f0d3d to
17b29d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agents-core/vision_agents/core/mcp/mcp_base.py (1)
84-93: Consider distinguishing connection errors from operation errors before disconnecting.The retry logic now disconnects and delays on every failure. This is appropriate for connection errors but may be overly aggressive for operation-specific errors (e.g., invalid tool name, bad arguments). Disconnecting for non-connection errors adds unnecessary reconnection overhead.
Consider catching specific connection-related exceptions separately, or checking if the error indicates a connection issue before triggering disconnect/reconnect.
Example approach:
async def _call_with_retry( self, operation_name: str, operation_func, *args, **kwargs ): """Call an MCP operation with auto-reconnect on failure.""" max_retries = 2 for attempt in range(max_retries + 1): try: await self._ensure_connected() await self._update_activity() return await operation_func(*args, **kwargs) except Exception as e: if attempt < max_retries: + # Only disconnect if error suggests connection issue + is_connection_error = isinstance(e, (ConnectionError, RuntimeError)) or \ + "Not connected" in str(e) self.logger.warning( f"{operation_name} failed (attempt {attempt + 1}/{max_retries + 1}): {e}. Reconnecting..." ) - await self.disconnect() - await asyncio.sleep(1) + if is_connection_error: + await self.disconnect() + await asyncio.sleep(1) else: self.logger.error( f"{operation_name} failed after {max_retries + 1} attempts: {e}" ) raise
🧹 Nitpick comments (5)
agents-core/vision_agents/core/mcp/mcp_base.py (2)
84-86: Redundant activity updates.Activity is updated here (line 85) and again inside each
_*_implmethod (lines 105, 116, 124, 133, 143, 154). While harmless, this creates duplicate timestamp updates for every operation.Consider removing the
_update_activity()calls from the impl methods since_call_with_retrynow handles it.
71-76: Add anasyncio.Lockto prevent concurrent_ensure_connected()calls and align with codebase patterns.Race condition confirmed: multiple concurrent operations can call
_ensure_connected()simultaneously, triggering concurrentconnect()calls. While concrete implementations (MCPServerRemote,MCPServerLocal) haveif self._is_connected:guards that prevent full reconnection, they don't prevent concurrent execution of setup code.Add a connection lock (following the pattern in
plugins/deepgram/vision_agents/plugins/deepgram/stt.py:115) to synchronize access:async def _ensure_connected(self) -> None: """Ensure the server is connected, reconnecting if necessary.""" async with self._connect_lock: if not self._is_connected: self.logger.info("Reconnecting to MCP server...") await self.connect()Initialize
self._connect_lock = asyncio.Lock()in__init__.agents-core/vision_agents/core/agents/agents.py (2)
68-77: Consider f-string formatting for consistency.The adapter uses
%formatting ("[Agent: %s] | %s" % (...)). While functional, f-strings would be more consistent with modern Python style. However, this is a minor style preference.def process(self, msg: str, kwargs): if self.extra: - return "[Agent: %s] | %s" % (self.extra["agent_id"], msg), kwargs + return f"[Agent: {self.extra['agent_id']}] | {msg}", kwargs return super(_AgentLoggerAdapter, self).process(msg, kwargs)
454-456: Consider consolidating multi-line log statements.Several log statements span multiple lines where a single-line call would be more concise. This is a minor style preference.
Example for lines 454-456:
- self.logger.info( - "🔚 Agent connection is already closed, finishing immediately" - ) + self.logger.info("🔚 Agent connection is already closed, finishing immediately")Also applies to: 708-710, 751-753
agents-core/pyproject.toml (1)
29-29: Consider adding major version upper bound to colorlog dependency.The specified version 6.10.1 is the latest stable release, so there's no currency or security concern. However, the permissive constraint
>=6.10.1is still worth refining. Adding an upper bound (e.g.,>=6.10.1,<7.0.0) would follow semantic versioning practices and provide protection against potential breaking changes in future major releases.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
examples/01_simple_agent_example/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
agents-core/pyproject.toml(1 hunks)agents-core/vision_agents/core/agents/agents.py(19 hunks)agents-core/vision_agents/core/events/manager.py(5 hunks)agents-core/vision_agents/core/mcp/mcp_base.py(2 hunks)agents-core/vision_agents/core/mcp/mcp_manager.py(3 hunks)agents-core/vision_agents/core/processors/base_processor.py(5 hunks)agents-core/vision_agents/core/utils/logging.py(2 hunks)plugins/cartesia/vision_agents/plugins/cartesia/tts.py(2 hunks)plugins/deepgram/vision_agents/plugins/deepgram/stt.py(3 hunks)plugins/elevenlabs/vision_agents/plugins/elevenlabs/tts.py(2 hunks)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py(8 hunks)plugins/kokoro/vision_agents/plugins/kokoro/tts.py(2 hunks)plugins/krisp/vision_agents/plugins/krisp/turn_detection.py(1 hunks)plugins/smart_turn/vision_agents/plugins/smart_turn/turn_detection.py(2 hunks)tests/test_utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- plugins/cartesia/vision_agents/plugins/cartesia/tts.py
- plugins/krisp/vision_agents/plugins/krisp/turn_detection.py
- plugins/kokoro/vision_agents/plugins/kokoro/tts.py
- agents-core/vision_agents/core/utils/logging.py
- plugins/deepgram/vision_agents/plugins/deepgram/stt.py
- agents-core/vision_agents/core/mcp/mcp_manager.py
- plugins/elevenlabs/vision_agents/plugins/elevenlabs/tts.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide
Files:
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.pyagents-core/vision_agents/core/agents/agents.pyagents-core/vision_agents/core/processors/base_processor.pytests/test_utils.pyagents-core/vision_agents/core/events/manager.pyplugins/smart_turn/vision_agents/plugins/smart_turn/turn_detection.pyagents-core/vision_agents/core/mcp/mcp_base.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
tests/**/*.py: Never use mocking utilities (e.g., unittest.mock, pytest-mock) in test files
Write tests using pytest (avoid unittest.TestCase or other frameworks)
Mark integration tests with @pytest.mark.integration
Do not use @pytest.mark.asyncio; async support is automatic
Files:
tests/test_utils.py
🧬 Code graph analysis (3)
agents-core/vision_agents/core/agents/agents.py (2)
agents-core/vision_agents/core/utils/logging.py (4)
CallContextToken(51-55)clear_call_context(97-104)set_call_context(84-94)configure_default_logging(20-47)agents-core/vision_agents/core/llm/realtime.py (2)
Realtime(23-201)_watch_video_track(68-70)
tests/test_utils.py (1)
agents-core/vision_agents/core/utils/logging.py (1)
configure_default_logging(20-47)
agents-core/vision_agents/core/mcp/mcp_base.py (1)
agents-core/vision_agents/core/mcp/mcp_manager.py (1)
call_tool(93-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
🔇 Additional comments (24)
plugins/smart_turn/vision_agents/plugins/smart_turn/turn_detection.py (3)
65-67: LGTM: Trailing comma improves diff readability.The trailing comma after
provider_nameis a good practice that makes future parameter additions cleaner in version control.
68-68: LGTM: Logger namespace change enables centralized configuration.Switching to
__name__-based logger aligns with the PR's objective to standardize logging and enable hierarchical configuration through the Agent class. The logger will now be part of thevision_agents.plugins.smart_turn.turn_detectionnamespace, allowing centralized control viaAgent(log_level=...).
100-100: LGTM: Single-line log format improves parseability.The single-line warning message is cleaner and easier to search in logs.
agents-core/vision_agents/core/events/manager.py (5)
356-358: LGTM! Appropriate verbosity reduction.Lowering handler registration from INFO to DEBUG is correct. This operational detail would be noisy during startup but remains available when debugging subscription issues.
365-367: LGTM! Appropriate verbosity reduction.When
ignore_unknown_events=True, skipped handlers are expected behavior. DEBUG level is correct for this diagnostic message.
403-403: LGTM! Appropriate verbosity reduction.Unregistered protobuf events are expected when
ignore_unknown_events=True. DEBUG level is suitable for this diagnostic message.
413-413: LGTM! Appropriate verbosity reduction.Consistent with other unregistered event handling. DEBUG level is correct for this diagnostic message.
510-512: LGTM! Appropriate verbosity reduction.Cancellation handling is internal state management during shutdown. DEBUG level is suitable while remaining available for troubleshooting shutdown behavior.
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (6)
33-34: LGTM! Module-level logger properly initialized.The logger initialization follows Python logging best practices and enables centralized configuration as described in the PR objectives.
112-113: Excellent use of DEBUG level for verbose internal filtering.Lowering this to DEBUG appropriately reduces noise during normal operations while preserving diagnostic value when troubleshooting.
119-122: INFO level appropriate for track lifecycle events.These significant state changes deserve INFO-level visibility. The timing information in the trackmap published message is particularly valuable for diagnosing synchronization issues.
Also applies to: 165-167
215-215: Log levels correctly distinguish expected vs. unexpected conditions.INFO for normal removal events and WARNING for missing track entries appropriately reflect the operational significance of each scenario.
Also applies to: 235-235
279-281: User-facing readiness messages appropriately logged at INFO level.The agent readiness indicators with emojis provide clear, visually distinctive signals in the logs for important state transitions.
Also applies to: 324-326
411-418: Perfect replacement of prints with appropriate logger calls.The log level progression (INFO → ERROR → WARNING) correctly models the browser opening flow, and the emojis enhance readability for users monitoring the logs.
agents-core/vision_agents/core/mcp/mcp_base.py (2)
20-20: LGTM! Module-level logger enables centralized configuration.The switch to
logging.getLogger(__name__)aligns with the PR's goal of allowing centralized logging configuration through the Agent class.
157-189: LGTM! Consistent delegation pattern.All public API methods consistently delegate to
_call_with_retry, ensuring uniform retry and reconnection behavior across all MCP operations.tests/test_utils.py (3)
1-12: LGTM - Clean import additions.The new imports (logging, pytest, configure_default_logging) are necessary for the logging tests and properly organized.
536-556: LGTM - Well-designed fixture for test isolation.The fixture properly captures and restores logger state, using list slicing (
handlers[:]) to create copies and avoid reference issues. This ensures clean test isolation.
558-594: LGTM - Comprehensive logging configuration tests.The test suite properly validates the three key behaviors of
configure_default_logging:
- Sets levels when unconfigured
- Preserves existing handlers
- Respects pre-configured log levels
Tests follow pytest guidelines without mocking or unittest.TestCase.
agents-core/vision_agents/core/agents/agents.py (3)
127-131: LGTM - Well-designed logging configuration parameter.The
log_levelparameter provides flexibility:
- Default
logging.INFOfor reasonable verbosityNoneoption allows users to manage logging externally- Conditional call to
configure_default_loggingrespects existing configThis aligns well with the PR objectives for centralized logging control.
176-176: LGTM - Appropriate log level reductions.Noisy operational logs (plugin registration, VAD events, video forwarder lifecycle) have been correctly lowered to DEBUG level, reducing noise in INFO logs while preserving diagnostic information.
Also applies to: 576-576, 860-860, 971-973
159-163: MCPManager logger parameter verification successful — code is correct.The MCPManager constructor at agents-core/vision_agents/core/mcp/mcp_manager.py line 14 does accept a
loggerparameter with proper type annotation (logging.Logger | logging.LoggerAdapter). The call on line 160 correctly passesself.loggeras the third positional argument. No issues.agents-core/vision_agents/core/processors/base_processor.py (2)
22-23: LGTM - Standard module-level logger pattern.The addition of
logger = logging.getLogger(__name__)follows Python logging best practices and enables centralized configuration through the new logging infrastructure.
158-158: LGTM - Consistent logging refactor.All
logging.infocalls have been correctly replaced withlogger.info, routing logs through the module-level logger for centralized configuration.Also applies to: 175-175, 187-187, 197-197
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agents-core/vision_agents/core/agents/agents.py (1)
534-548: Remove duplicate connection check.The early exit guard at lines 534-539 correctly checks if
_connectionis None and returns immediately. However, lines 542-548 contain a duplicate check that is now unreachable. Additionally, line 545 useslogging.infoinstead ofself.logger.info, creating inconsistency.Apply this diff to remove the duplicate code:
# If connection is None or already closed, return immediately if not self._connection: self.logger.info( "🔚 Agent connection is already closed, finishing immediately" ) return - with self.span("agent.finish"): - # If connection is None or already closed, return immediately - if not self._connection: - logging.info( - "🔚 Agent connection already closed, finishing immediately" - ) - return - @self.edge.events.subscribe async def on_ended(event: CallEndedEvent): self._is_running = False
♻️ Duplicate comments (1)
agents-core/vision_agents/core/agents/agents.py (1)
177-177: Agent ID initialization timing issue persists.The logger adapter is created with
self.agent_user.id, which defaults to an empty string at this point in initialization. The ID is only assigned later increate_user()(line 661), called fromjoin()(line 460). Since the adapter'sextradict is immutable after construction, all logs—including those beforejoin()—will displayagent_id="".Consider generating and assigning a temporary or final UUID to
self.agent_user.idearlier in__init__(e.g., lines 169-170, right after settingself.agent_user) when it's empty, ensuring the adapter captures a meaningful ID from the start.
🧹 Nitpick comments (3)
plugins/krisp/vision_agents/plugins/krisp/turn_detection.py (3)
30-31: Consider routing Krisp SDK logs through the logging module.This callback uses
print()directly, which is inconsistent with the PR's goal of removing direct prints. Consider adapting it to uselogging.getLogger(__name__)instead:def log_callback(log_message, log_level): - print(f"[{log_level}] {log_message}", flush=True) + logger = logging.getLogger(__name__) + logger.debug(f"[Krisp-{log_level}] {log_message}")This would integrate Krisp's internal logging with the centralized logging configuration.
148-148: Consider lowering this log to DEBUG level to reduce noise.This logs every turn score for each processed frame, which could generate significant log volume during active audio processing. The PR aims to lower noisy messages to DEBUG level.
- self.logger.info(f"Score from turn {score}") + self.logger.debug(f"Score from turn {score}")
57-72: Add docstrings to methods per coding guidelines.Several methods lack docstrings, which are required by the project's coding guidelines. Consider adding Google-style docstrings for
_initialize_krispandprocess_pcm_turn_takingto document their purpose, parameters, exceptions, and behavior.As per coding guidelines.
Also applies to: 132-181
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
agents-core/pyproject.toml(1 hunks)agents-core/vision_agents/core/agents/agents.py(18 hunks)plugins/cartesia/vision_agents/plugins/cartesia/tts.py(2 hunks)plugins/elevenlabs/vision_agents/plugins/elevenlabs/tts.py(2 hunks)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py(8 hunks)plugins/kokoro/vision_agents/plugins/kokoro/tts.py(2 hunks)plugins/krisp/vision_agents/plugins/krisp/turn_detection.py(1 hunks)tests/test_utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/kokoro/vision_agents/plugins/kokoro/tts.py
- plugins/cartesia/vision_agents/plugins/cartesia/tts.py
- plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py
- plugins/elevenlabs/vision_agents/plugins/elevenlabs/tts.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide
Files:
plugins/krisp/vision_agents/plugins/krisp/turn_detection.pyagents-core/vision_agents/core/agents/agents.pytests/test_utils.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
tests/**/*.py: Never use mocking utilities (e.g., unittest.mock, pytest-mock) in test files
Write tests using pytest (avoid unittest.TestCase or other frameworks)
Mark integration tests with @pytest.mark.integration
Do not use @pytest.mark.asyncio; async support is automatic
Files:
tests/test_utils.py
🧠 Learnings (1)
📚 Learning: 2025-10-13T22:00:34.300Z
Learnt from: dangusev
PR: GetStream/Vision-Agents#98
File: plugins/deepgram/vision_agents/plugins/deepgram/stt.py:135-150
Timestamp: 2025-10-13T22:00:34.300Z
Learning: In the Deepgram STT plugin (plugins/deepgram/vision_agents/plugins/deepgram/stt.py), the `started()` method is designed to wait for the connection attempt to complete, not to guarantee a successful connection. It's acceptable for the connection attempt to fail, and downstream code handles the case where `self.dg_connection` is `None`. The `_connected_once` event is set in the `finally` block intentionally to signal attempt completion.
Applied to files:
agents-core/vision_agents/core/agents/agents.py
🧬 Code graph analysis (2)
agents-core/vision_agents/core/agents/agents.py (3)
agents-core/vision_agents/core/utils/logging.py (4)
CallContextToken(51-55)clear_call_context(97-104)set_call_context(84-94)configure_default_logging(20-47)agents-core/vision_agents/core/llm/realtime.py (2)
Realtime(24-203)_watch_video_track(70-72)agents-core/vision_agents/core/utils/video_utils.py (1)
ensure_even_dimensions(6-26)
tests/test_utils.py (1)
agents-core/vision_agents/core/utils/logging.py (1)
configure_default_logging(20-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
🔇 Additional comments (11)
plugins/krisp/vision_agents/plugins/krisp/turn_detection.py (1)
45-45: LGTM! Logger now uses module name for centralized configuration.This change aligns perfectly with the PR's objective of making loggers configurable from the Agent class via module hierarchy rather than hardcoded logger names.
agents-core/pyproject.toml (1)
29-29: LGTM! Clean dependency swap for logging infrastructure.The addition of
colorlog>=6.10.1supports the new colored logging formatter introduced in this PR, replacing the removed torchvision dependency.tests/test_utils.py (3)
1-13: LGTM! Imports support the new logging tests.The added imports for
logging,pytest, andconfigure_default_loggingare appropriate for testing the new logging infrastructure.
553-573: LGTM! Well-designed fixture for logger state isolation.The
make_loggerfixture properly captures and restores logger state, preventing test pollution. The factory pattern allows testing multiple loggers in a single test.
575-611: LGTM! Comprehensive test coverage for logging configuration.The three test methods thoroughly verify
configure_default_loggingbehavior: configuring unconfigured loggers, preserving existing handlers, and preserving pre-set levels. Tests follow pytest guidelines and are well-structured.agents-core/vision_agents/core/agents/agents.py (6)
18-23: LGTM! Import updates support the logging infrastructure.The added imports for logging utilities (
CallContextToken,clear_call_context,set_call_context,configure_default_logging) and reformatted edge event imports align with the PR's logging refactor objectives.Also applies to: 42-47
78-86: LGTM! Logger adapter design is sound.The
_AgentLoggerAdapterclass correctly extendsLoggerAdapterto inject agent context into log messages. The implementation properly formats messages with the agent ID.
155-160: LGTM! Log level parameter enables user control.The
log_levelparameter with a default oflogging.INFOallows users to configure SDK logging at initialization, and passingNonelets users manage logging configuration themselves. This design aligns well with the PR objectives.
213-213: LGTM! Appropriate log level adjustments.The changes to use
debuglevel for plugin registration (line 213), VAD events (line 668), and VideoForwarder creation (line 955) appropriately reduce log verbosity for internal operations while keeping important user-facing events atinfolevel.Also applies to: 668-668, 955-955
273-273: LGTM! Correct tracer usage.Using
self.tracer(initialized from the constructor parameter at line 173) instead of the module-leveltraceris appropriate and enables tracer injection for testing.
837-848: LGTM! Formatting improvements enhance readability.The reformatted conditional checks (lines 837-848), track iteration logic (lines 888-912), and tuple assignments (lines 961-965) improve code clarity without changing functionality.
Also applies to: 888-912, 961-965
commit d78a4a0 Author: Dan Gusev <dangusev92@gmail.com> Date: Thu Oct 30 20:44:19 2025 +0100 Logging cleanup (#133) * logging: use logging.getLogger(__name__) everywhere to simplify configuration * Clean up logging everywhere - Replaced "logging.info" usages with separate loggers - Lowered some info messages to debug - Replaced prints with logging - Added emojis where they're already used * Enable default logging for the SDK logs - Added a way to set the SDK log level at the Agent class - Set the default formatter and added level-based coloring - If the logs are already configured, they remain intact - Moved logging_utils.py to utils/logging.py * Remove "name" from the default logging formatter --------- Co-authored-by: Thierry Schellenbach <thierry@getstream.io>
* Add AWS Bedrock function calling implementation - Implemented function calling for AWS Bedrock Realtime (Nova Sonic) - Added tool schema conversion to AWS Nova format - Implemented tool execution handlers for realtime API - Added audio resampling fix for simple_audio_response - Created example demonstrating function calling with AWS LLM - Updated README with function calling documentation - Added test for AWS Realtime function calling Note: AWS Nova Realtime toolConfiguration causes connection errors, likely an API limitation. Implementation is ready for when AWS adds support. * Update converse format. Verbose logging * Clean up for LLM * Working realtime function calling (pre cleanup) * Squashed commit of the following: commit d78a4a0 Author: Dan Gusev <dangusev92@gmail.com> Date: Thu Oct 30 20:44:19 2025 +0100 Logging cleanup (#133) * logging: use logging.getLogger(__name__) everywhere to simplify configuration * Clean up logging everywhere - Replaced "logging.info" usages with separate loggers - Lowered some info messages to debug - Replaced prints with logging - Added emojis where they're already used * Enable default logging for the SDK logs - Added a way to set the SDK log level at the Agent class - Set the default formatter and added level-based coloring - If the logs are already configured, they remain intact - Moved logging_utils.py to utils/logging.py * Remove "name" from the default logging formatter --------- Co-authored-by: Thierry Schellenbach <thierry@getstream.io> * Lint and formatting after merge * Remove resample_audio in favor of PCM.resample * Migrate RealtimeAudioOutputEvent to _emit_audio_output_event * unused import * Fix linting: remove unused imports * Clean up excessive logging in AWS Bedrock realtime - Reduce connection setup logs from 7+ to 2 INFO messages - Change most event handling logs to DEBUG level instead of INFO - Remove duplicate 'Response processing error' log message - Remove unused _pending_tool_uses dictionary - Remove redundant debug logs in send_raw_event * Fix mypy type errors in AWS LLM plugin - Add type annotation for tool_calls list - Use cast() to properly type NormalizedToolCallItem lists for _dedup_and_execute - Import cast from typing module --------- Co-authored-by: Neevash Ramdial (Nash) <mail@neevash.dev>
What's changed
Cleaned up logging across the repo and added a way to configure SDK logs (i.e.
vision_agentsandgetstreamloggers) viaAgentclass.__name__so they're configurable from one placeMain change to look at - the
Agentcan now configure the log level forvision_agentsandgetstreamloggers.This way users can change it from one place, and we can set the default level to
INFO.If the level OR handlers for these loggers are already configured, they will remain as is.
Also, an example of the new logging format:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests