fix(together): record failed spans on API errors#4083
Open
muhamedfazalps wants to merge 2 commits intotraceloop:mainfrom
Open
fix(together): record failed spans on API errors#4083muhamedfazalps wants to merge 2 commits intotraceloop:mainfrom
muhamedfazalps wants to merge 2 commits intotraceloop:mainfrom
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Together AI instrumentor adds explicit exception handling to the ChangesTogether AI Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-together/tests/test_errors.py (2)
30-39: ⚡ Quick win
span.endassertion missing, and prefer direct enum comparison over.name.Two small gaps in
test_chat_api_error_records_exception_message:
span.end.assert_called_once()is not asserted here — test 1 covers it for a different error type, but since this test independently constructs a new span mock it should verify the span is also ended for completeness.- Comparing
status_arg.status_code.name == "ERROR"via string is fragile; comparing directly against the importedStatusCodeenum is more idiomatic and immune to name changes.🔧 Proposed improvement
+from opentelemetry.trace.status import StatusCode + def test_chat_api_error_records_exception_message(): span, wrapper = _make_wrapper() wrapped = MagicMock(side_effect=ValueError("bad request")) with pytest.raises(ValueError, match="bad request"): wrapper(wrapped, None, [], {"model": "test-model"}) status_arg = span.set_status.call_args.args[0] - assert status_arg.status_code.name == "ERROR" + assert status_arg.status_code == StatusCode.ERROR assert status_arg.description == "bad request" + span.end.assert_called_once()🤖 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-together/tests/test_errors.py` around lines 30 - 39, The test test_chat_api_error_records_exception_message is missing an assertion that the span was ended and uses a fragile string comparison for the status code; update the test to assert span.end.assert_called_once() (or span.end.assert_called_once_with() if specific args) after the error handling, and replace the string comparison status_arg.status_code.name == "ERROR" with a direct enum comparison using the imported StatusCode (e.g., status_arg.status_code == StatusCode.ERROR) so the test checks end() was invoked and uses the stable StatusCode enum.
25-25: 💤 Low valueStrengthen
record_exceptionassertion to verify the exception object.
assert_called_once()confirms the call was made but not that the right exception was passed. Consider usingassert_called_once_with(ANY)or capturing the argument and checking its message:🔧 Proposed improvement
- span.record_exception.assert_called_once() + exc_arg = span.record_exception.call_args.args[0] + assert isinstance(exc_arg, RuntimeError) + assert "401 Unauthorized" in str(exc_arg)🤖 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-together/tests/test_errors.py` at line 25, The test currently only checks that span.record_exception was called (span.record_exception.assert_called_once()); strengthen it by verifying the actual exception argument: replace or augment that assertion with span.record_exception.assert_called_once_with(ANY) (importing unittest.mock.ANY) or capture the call args (via span.record_exception.call_args) and assert the exception instance/message matches the expected error string; reference span.record_exception, assert_called_once_with, ANY or use span.record_exception.call_args to inspect the passed 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.
Nitpick comments:
In `@packages/opentelemetry-instrumentation-together/tests/test_errors.py`:
- Around line 30-39: The test test_chat_api_error_records_exception_message is
missing an assertion that the span was ended and uses a fragile string
comparison for the status code; update the test to assert
span.end.assert_called_once() (or span.end.assert_called_once_with() if specific
args) after the error handling, and replace the string comparison
status_arg.status_code.name == "ERROR" with a direct enum comparison using the
imported StatusCode (e.g., status_arg.status_code == StatusCode.ERROR) so the
test checks end() was invoked and uses the stable StatusCode enum.
- Line 25: The test currently only checks that span.record_exception was called
(span.record_exception.assert_called_once()); strengthen it by verifying the
actual exception argument: replace or augment that assertion with
span.record_exception.assert_called_once_with(ANY) (importing unittest.mock.ANY)
or capture the call args (via span.record_exception.call_args) and assert the
exception instance/message matches the expected error string; reference
span.record_exception, assert_called_once_with, ANY or use
span.record_exception.call_args to inspect the passed exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1e8a54f-4fac-4992-a1a9-4763b0be065b
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-together/opentelemetry/instrumentation/together/__init__.pypackages/opentelemetry-instrumentation-together/tests/test_errors.py
|
muhamedfazalps seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Related to #412. This keeps the fix scoped to the Together instrumentation package, matching the maintainer preference for small PRs.
Summary by CodeRabbit
Bug Fixes
Tests