fix(vertexai): make dont_throw work on async functions - #4415
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe VertexAI ChangesVertexAI exception suppression
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The change is localized to async error handling with dedicated tests, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-vertexai/tests/test_dont_throw.py (1)
7-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the configured exception callback.
The tests verify suppression and return values, but they do not configure
Config.exception_logger. Add synchronous and asynchronous cases that verify the callback receives the original exception. Add a callback-that-raises case to verify thatdont_throwstill returnsNone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opentelemetry-instrumentation-vertexai/tests/test_dont_throw.py` around lines 7 - 26, Add synchronous and asynchronous tests around dont_throw that set Config.exception_logger, assert it receives the original raised exception, and restore the configuration afterward. Also add a case where the configured callback raises, verifying both sync and async dont_throw calls still return None without propagating either exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/utils.py`:
- Around line 29-36: Update _handle_exception so Config.exception_logger(e) is
invoked within its own suppression boundary; catch any exception raised by the
callback, log the callback failure through logger.debug, and do not re-raise it,
preserving the guarantee that instrumentation errors cannot escape to the Vertex
AI call.
---
Nitpick comments:
In `@packages/opentelemetry-instrumentation-vertexai/tests/test_dont_throw.py`:
- Around line 7-26: Add synchronous and asynchronous tests around dont_throw
that set Config.exception_logger, assert it receives the original raised
exception, and restore the configuration afterward. Also add a case where the
configured callback raises, verifying both sync and async dont_throw calls still
return None without propagating either exception.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d72bac5d-8fb6-4dbb-b515-3122e5ccd7ec
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/utils.pypackages/opentelemetry-instrumentation-vertexai/tests/test_dont_throw.py
f40fd2d to
af1460e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opentelemetry-instrumentation-vertexai/tests/test_dont_throw.py`:
- Around line 7-12: Extend the tests for dont_throw in
test_dont_throw_swallows_sync_exceptions and the corresponding asynchronous test
to capture debug logging and assert that each swallowed exception emits the
expected log through _handle_exception. Preserve the existing assertions that
synchronous and asynchronous exceptions do not reach the caller.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6865caed-9b9c-47bc-bda7-1677cf9e3a5a
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/utils.pypackages/opentelemetry-instrumentation-vertexai/tests/test_dont_throw.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/utils.py
dont_throwin the VertexAI instrumentation defines only a synchronous wrapper. Calling anasync defreturns a coroutine immediately, so thetryblock exits before the function body runs and the caller awaits the coroutine outside the guard. The decorator has no effect on async functions.It is applied to async functions in this package:
span_utils.set_input_attributesand_handle_request._handle_requestis awaited at__init__.pywith no surrounding try, and before the wrapped model call, so an instrumentation error in async prompt capture propagates into the user's application and the LLM call never happens.Before:
After:
The Anthropic, OpenAI and google-generativeai packages already branch on
asyncio.iscoroutinefunctionand return an async wrapper. This copies that shape into VertexAI.Cohere has the same gap, but its async streaming entry points are async generators, which need a third branch. Left out to keep this focused.
Tests added in
tests/test_dont_throw.pycover both wrappers, the exception path and the return-value path. They fail on main for the async cases.Fixes #4414
Summary by CodeRabbit
Bug Fixes
Tests