-
Notifications
You must be signed in to change notification settings - Fork 808
fix(bedrock): handle non-text contentBlockDelta events in converse_stream #3404
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
- citation - reasoningContent - toolUse - text https://docs.aws.amazon.com/bedrock/latest/APIReference/API_runtime_ContentBlockDeltaEvent.html So we can't assume that text will always exist within a event["contentBlockDelta"]["delta"] object, as this is not the case.
Add test to validate handling of non-text contentBlockDelta events in Bedrock's converse_stream API. According to AWS documentation, contentBlockDelta can contain one of: citation, reasoningContent, toolUse, or text. The new test `test_anthropic_converse_stream_with_tool_use` ensures that when the model uses tools, the instrumentation correctly handles contentBlockDelta events that contain toolUse instead of text, preventing KeyError exceptions. Changes: - Add test_anthropic_converse_stream_with_tool_use in test_anthropic.py - Record VCR cassette with tool use interaction - Clean cassette to remove AWS credentials and security tokens This test validates the fix in commit b84cd7c where the code was updated to check for text existence before accessing it: if "contentBlockDelta" in event and "text" in event["contentBlockDelta"].get("delta", {})
|
WalkthroughAdds a guard in Bedrock instrumentation to append streaming text only when present in contentBlockDelta.delta.text. Introduces a new test for Anthropic converse_stream with tool use and a corresponding cassette, validating handling of non-text deltas and ensuring spans and attributes are recorded. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test as Test
participant Client as Bedrock Runtime Client
participant Bedrock as Bedrock Service
participant Instr as Instrumentation
rect rgb(240,245,255)
Test->>Client: converse_stream(request with toolConfig)
Client->>Bedrock: HTTP POST /model/... (stream)
Bedrock-->>Client: EventStream (contentBlockDelta / toolUse / text / ...)
loop For each event
Client-->>Instr: on_event(event)
alt Has contentBlockDelta.delta.text
Instr->>Instr: append text to response_msg
else Non-text delta (e.g., toolUse)
Instr->>Instr: skip text accumulation
end
end
end
Client-->>Test: Stream iteration completes
Instr-->>Instr: record span with model/vendor/request type
note over Instr: No legacy attributes logs emitted
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (2)**/cassettes/**/*.{yaml,yml,json}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)packages/opentelemetry-instrumentation-bedrock/tests/traces/test_anthropic.py (2)
🪛 Ruff (0.13.3)packages/opentelemetry-instrumentation-bedrock/tests/traces/test_anthropic.py982-982: Unused function argument: (ARG001) 🔇 Additional comments (4)
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 d511cd6 in 47 seconds. Click for details.
- Reviewed
248
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
draft 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-bedrock/opentelemetry/instrumentation/bedrock/__init__.py:391
- Draft comment:
Nice fix: the updated conditional checks if the 'delta' field contains a 'text' key before appending, which prevents KeyError on non-text contentBlockDelta events. This aligns with the AWS Bedrock API documentation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it praises the fix and explains what it does without suggesting any changes or improvements. It doesn't ask for confirmation or suggest any specific action.
2. packages/opentelemetry-instrumentation-bedrock/tests/traces/test_anthropic.py:980
- Draft comment:
The new test 'test_anthropic_converse_stream_with_tool_use' effectively validates that non-text (e.g. toolUse) events do not crash the instrumentation and correctly set span attributes. This improves coverage of edge cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, describing what the new test does and its benefits. It doesn't provide any actionable feedback or suggestions for improvement.
Workflow ID: wflow_qQ6LlCPvhh9sBvsQ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
feat(instrumentation): ...
orfix(instrumentation): ...
.Fixes #2961
This PR fixes the KeyError when using the bedrock converse api with streaming, on events other than text, and adds a test to validate the fix for handling non-text
contentBlockDelta
events in Bedrock'sconverse_stream
API.Background
According to AWS Bedrock API documentation, the
contentBlockDelta
field can contain EITHER of the following:citation
reasoningContent
toolUse
text
The previous code assumed
text
would always be present, causing KeyError exceptions when the model uses tools or returns other content types.Changes
test_anthropic_converse_stream_with_tool_use
intest_anthropic.py
Test Details
The new test:
get_weather
tool definitioncontentBlockDelta
events withtoolUse
(instead oftext
) don't crash the instrumentationRelated
Important
Fixes KeyError in
converse_stream
by handling non-textcontentBlockDelta
events and adds a test to validate the fix.converse_stream
in__init__.py
by checking for 'text' incontentBlockDelta
events.toolUse
without crashing.test_anthropic_converse_stream_with_tool_use
intest_anthropic.py
to validate handling of non-textcontentBlockDelta
events.test_anthropic_converse_stream_with_tool_use.yaml
to simulate tool use interaction.use_legacy_attributes
is True.This description was created by
for d511cd6. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests