-
Notifications
You must be signed in to change notification settings - Fork 824
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
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", {})
|
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure 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)
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
248lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2draft 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.
nirga
left a 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.
Thanks @octavifs!
|
You're welcome! |
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
contentBlockDeltaevents in Bedrock'sconverse_streamAPI.Background
According to AWS Bedrock API documentation, the
contentBlockDeltafield can contain EITHER of the following:citationreasoningContenttoolUsetextThe previous code assumed
textwould always be present, causing KeyError exceptions when the model uses tools or returns other content types.Changes
test_anthropic_converse_stream_with_tool_useintest_anthropic.pyTest Details
The new test:
get_weathertool definitioncontentBlockDeltaevents withtoolUse(instead oftext) don't crash the instrumentationRelated
Important
Fixes KeyError in
converse_streamby handling non-textcontentBlockDeltaevents and adds a test to validate the fix.converse_streamin__init__.pyby checking for 'text' incontentBlockDeltaevents.toolUsewithout crashing.test_anthropic_converse_stream_with_tool_useintest_anthropic.pyto validate handling of non-textcontentBlockDeltaevents.test_anthropic_converse_stream_with_tool_use.yamlto simulate tool use interaction.use_legacy_attributesis 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