-
Notifications
You must be signed in to change notification settings - Fork 872
fix(vertexai): add error logging to instrumented functions #3624
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
base: main
Are you sure you want to change the base?
Conversation
Adds error handling to the sync and async wrappers in vertexai instrumentation to properly log errors via OTel spans when API calls fail. Changes: - Set ERROR_TYPE attribute with exception class name - Record exception on span - Set span status to ERROR with message Part of traceloop#412 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds centralized error handling to Vertex AI instrumentation: exceptions caught in sync/async streaming and wrapper paths now set an ERROR_TYPE span attribute, record the exception, set span status to ERROR, end the span, and re-raise; successful paths unchanged. (≤50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 9272570 in 1 minute and 50 seconds. Click for details.
- Reviewed
44lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/__init__.py:249
- Draft comment:
Good use of try/except in _awrap. Capturing exceptions with ERROR_TYPE, recording the error, updating status, ending the span, then re-raising preserves expected behavior. Consider if exposing str(e) in span status is acceptable. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% The comment says "Consider if exposingstr(e)in span status is acceptable." This is asking the PR author to think about whether this is okay, rather than stating that there is definitely a problem. It's a speculative comment that doesn't provide clear evidence of an issue. The comment doesn't say "This will expose sensitive information" or "Remove str(e) from the span status" - it just asks the author to consider it. This violates the rule about not asking the PR author to confirm their intention or double-check things. The comment is not actionable because it doesn't suggest a specific code change. However, this could be a legitimate security concern about potentially exposing sensitive information in exception messages. If exception messages could contain PII or sensitive data, this would be a real issue. The comment might be trying to raise awareness of a security best practice. While security concerns are valid, the comment doesn't provide any evidence that this is actually a problem in this specific context. It's purely speculative ("Consider if...") rather than stating a definite issue. Without concrete evidence that Vertex AI exceptions contain sensitive data, or a clear recommendation to change the code, this is just asking the author to think about it, which violates the rules. This comment should be deleted because it's speculative and asks the author to consider something without providing clear evidence of a problem or a specific code change. It violates the rule against asking the PR author to confirm, verify, or ensure things.
2. packages/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/__init__.py:310
- Draft comment:
The try/except in _wrap similarly ensures errors are captured on synchronous calls. Recording ERROR_TYPE, exception details, and setting ERROR status before ending the span follows best practices. Verify that exposing str(e) is acceptable for your use case. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify if exposingstr(e)is acceptable, which falls under asking the author to confirm their intention or ensure behavior is intended. This violates the rules.
3. packages/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/__init__.py:32
- Draft comment:
Typographical suggestion: The content on line 32 appears to consist solely of a ')'. If this is unintended, please remove or correct it. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_2A2AFRrdybQ3m0BF
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/__init__.py`:
- Around line 249-256: The current try/except only wraps the initial await
wrapped(*args, **kwargs) so streaming iteration errors bypass span handling;
update the streaming handlers (_build_from_streaming_response and
_abuild_from_streaming_response) to wrap their for and async for loops in
try/except blocks that mirror the outer handler: on exception set
span.set_attribute(ERROR_TYPE, e.__class__.__name__), span.record_exception(e),
span.set_status(Status(StatusCode.ERROR, str(e))), call span.end(), then
re-raise the exception so iteration-level errors are captured and the span is
properly closed.
...es/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/__init__.py
Show resolved
Hide resolved
Addresses CodeRabbit review feedback - the streaming iteration loops also need try/except to capture errors that occur during streaming. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/__init__.py`:
- Line 193: The async streaming call incorrectly passes the generator object
`response` to `handle_streaming_response`; change the call to pass the
accumulated string `complete_response` instead so the span receives the final
text. Update the invocation of handle_streaming_response(span, event_logger,
llm_model, response, token_usage) to use handle_streaming_response(span,
event_logger, llm_model, complete_response, token_usage) in the async flow to
match the sync behavior.
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/__init__.py (1)
149-172: Error handling implementation looks correct.The try/except properly wraps the iteration loop, capturing streaming exceptions with appropriate span attributes, exception recording, status setting, and span termination before re-raising.
Minor observation:
handle_streaming_response(line 167) already callsspan.set_status(Status(StatusCode.OK))when the span is recording (see line 146), making line 171 redundant. This is harmless but could be cleaned up for clarity.
...es/opentelemetry-instrumentation-vertexai/opentelemetry/instrumentation/vertexai/__init__.py
Outdated
Show resolved
Hide resolved
Fixes bug in async streaming handler where `response` (generator) was passed instead of `complete_response` (accumulated string). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Adds error handling to the vertexai instrumentation so exceptions are properly recorded on OTel spans.
Changes
ERROR_TYPEfrom semconv error attributes_awrap(async) wrapper_wrap(sync) wrapperValidation
Testing
Part of #412
Important
Add error handling to Vertex AI instrumentation by logging exceptions in
_awrapand_wrapfunctions._awrapand_wrapto log exceptions.ERROR_TYPE, record exception, set status toERROR, end span.ERROR_TYPEfromsemconv.error_attributes.This description was created by
for 9272570. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.