fix(anthropic): move beta AsyncMessages.stream to sync wrapper #3531
Conversation
The beta streaming API methods (beta.messages.AsyncMessages.stream)
were incorrectly placed in WRAPPED_AMETHODS, causing them to be
wrapped with an async wrapper that awaits the result. However,
stream() returns an async context manager, not a coroutine, so it
should use the sync wrapper like the non-beta version.
This fixes the error:
RuntimeWarning: coroutine '_awrap' was never awaited
TypeError: 'coroutine' object does not support the asynchronous
context manager protocol
Applies the same fix from PR traceloop#3220 to the beta API endpoints that
were missed.
Fixes traceloop#3178
Signed-off-by: Irving Popovetsky <irving@honeycomb.io>
|
|
📝 WalkthroughWalkthroughMoves streaming handling for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 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 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to c26aee3 in 36 seconds. Click for details.
- Reviewed
270lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft 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-anthropic/opentelemetry/instrumentation/anthropic/__init__.py:97
- Draft comment:
Good: The beta AsyncMessages.stream entry has been added to WRAPPED_METHODS. This change ensures that the method is wrapped using the sync wrapper, which is the correct behavior for an async context manager. This aligns with the non-beta API fix and resolves #3178. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what the change does and why it was made. It doesn't ask for any specific action or suggest any improvements. According to the rules, purely informative comments should be removed.
2. packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.py:110
- Draft comment:
Nice: The beta AsyncMessages.stream entry for the Bedrock SDK ('anthropic.lib.bedrock._beta_messages') has been added to WRAPPED_METHODS. This consistently applies the fix for both beta APIs. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.py:132
- Draft comment:
Removal: Taking the beta AsyncMessages.stream entries out of WRAPPED_AMETHODS avoids the erroneous async wrapping (which would await a context manager). This change prevents the TypeError and RuntimeWarning observed earlier. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. packages/opentelemetry-instrumentation-anthropic/tests/test_beta_streaming.py:22
- Draft comment:
The new tests for the beta streaming API are comprehensive. They cover both sync and async usage, verify span attributes and log outputs for legacy and event-enabled scenarios, which is excellent for guarding against regressions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, praising the comprehensiveness of the tests without suggesting any changes or improvements. It doesn't align with the rules as it doesn't provide actionable feedback or suggestions.
Workflow ID: wflow_ft1EVXQYqGof3BHM
You can customize 
![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)
packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.py (1)
87-99: Consider adding an explanatory comment for the beta methods.Similar to the comment on lines 78-80 that explains why non-beta
AsyncMessages.streamuses the sync wrapper, consider adding a comment before the beta method entries (lines 100-105 and 119-124) to clarify that these async resources also return async context managers and therefore use the sync wrapper.🔎 Proposed addition
}, + # Beta API methods use the same pattern: AsyncMessages.stream returns + # an async context manager, so we use the sync wrapper (not async wrapper) { "package": "anthropic.resources.beta.messages.messages", "object": "AsyncMessages",
📜 Review details
Configuration used: defaults
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 selected for processing (2)
packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.pypackages/opentelemetry-instrumentation-anthropic/tests/test_beta_streaming.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-anthropic/tests/test_beta_streaming.pypackages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.py
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-anthropic/tests/test_beta_streaming.py (1)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
SpanAttributes(64-245)
🪛 Ruff (0.14.10)
packages/opentelemetry-instrumentation-anthropic/tests/test_beta_streaming.py
24-24: Unused function argument: instrument_legacy
(ARG001)
24-24: Unused function argument: reader
(ARG001)
76-76: Unused function argument: instrument_legacy
(ARG001)
76-76: Unused function argument: reader
(ARG001)
136-136: Unused function argument: instrument_with_content
(ARG001)
136-136: Unused function argument: reader
(ARG001)
173-173: Unused function argument: instrument_with_no_content
(ARG001)
177-177: Unused function argument: reader
(ARG001)
🔇 Additional comments (6)
packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.py (2)
100-105: LGTM! Beta AsyncMessages.stream correctly uses sync wrapper.This change correctly moves the beta
AsyncMessages.streammethod toWRAPPED_METHODS, matching the pattern established for the non-beta version (lines 81-86). Sincestream()returns an async context manager rather than a coroutine, it must not be awaited by the async wrapper.
119-124: LGTM! Bedrock beta AsyncMessages.stream correctly uses sync wrapper.This change correctly applies the same fix to the Bedrock beta variant, ensuring consistency across all beta streaming APIs.
packages/opentelemetry-instrumentation-anthropic/tests/test_beta_streaming.py (4)
22-71: LGTM! Comprehensive test for sync beta streaming.The test correctly validates sync beta streaming behavior with legacy attributes, including prompt/completion content, token usage, and log emission. The test structure and assertions are appropriate.
Note: The Ruff warnings about unused
instrument_legacyandreaderarguments are false positives—these are pytest fixtures used for their side effects (instrumentation setup and metrics collection).
73-131: LGTM! Critical test case validates the fix.This is the main test that validates the fix for issue #3178. Before the fix, this would fail with "TypeError: 'coroutine' object does not support the asynchronous context manager protocol." The test now passes because
AsyncMessages.streamuses the sync wrapper instead of the async wrapper.The docstring clearly explains the problem and solution, making this an excellent regression test.
Note: The Ruff warnings about unused arguments are false positives for pytest fixtures.
133-168: LGTM! Test validates event-based logging with content.The test correctly validates beta streaming with events and content logging enabled, verifying that 2 logs are emitted and token usage is tracked properly. The focus on token usage rather than span attributes is appropriate for event-based instrumentation.
Note: The Ruff warnings about unused arguments are false positives for pytest fixtures.
170-208: LGTM! Test validates event-based logging without content.The test correctly validates that beta streaming with events still emits logs (for structure) even when content logging is disabled. This complements the previous test by covering a different instrumentation configuration.
Note: The Ruff warnings about unused arguments are false positives for pytest fixtures.
2 participants
Summary
PR #3220 fixed
AsyncMessages.stream()for the non-beta API by moving it fromWRAPPED_AMETHODStoWRAPPED_METHODS. However, the beta API equivalents were not included in that fix.This PR applies the same fix to:
anthropic.resources.beta.messages.messages.AsyncMessages.streamanthropic.lib.bedrock._beta_messages.AsyncMessages.streamProblem
When using
async with client.beta.messages.stream(...), the instrumentation fails with:Reproduction Environment
Root Cause
stream()returns anAsyncMessageStreamManager(async context manager), not a coroutine. The async wrapper incorrectly awaits it, breaking the context manager protocol.Solution
Move the beta
AsyncMessages.streamentries fromWRAPPED_AMETHODStoWRAPPED_METHODS, matching the pattern established in PR #3220 for the non-beta version.Testing
Related
feat(instrumentation): ...orfix(instrumentation): ....Important
Fixes beta API
AsyncMessages.streamby moving it to sync wrapper, ensuring correct context manager usage.AsyncMessages.streamfor beta API fromWRAPPED_AMETHODStoWRAPPED_METHODSin__init__.py.AsyncMessages.streamuses sync wrapper, fixing context manager protocol issue.test_beta_streaming.pyto test beta streaming API with legacy attributes, events, and content logging.This description was created by
for c26aee3. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.