fix(ollama): record failed spans when API raises exceptions#4127
Open
Charan-place wants to merge 2 commits into
Open
fix(ollama): record failed spans when API raises exceptions#4127Charan-place wants to merge 2 commits into
Charan-place wants to merge 2 commits into
Conversation
When `Client._request` or `AsyncClient._request` raises, the span was left without an error status and was never ended. Wrap both `_wrap` and `_awrap` API calls in try/except so exceptions are captured via `set_status(ERROR)`, `record_exception`, and `span.end()` before re-raising. Fixes traceloop#412 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request adds exception handling across synchronous and asynchronous Ollama instrumentation paths: wrappers and streaming accumulators now catch errors, set span StatusCode.ERROR with the exception message, record exception events, end spans, and re-raise. Tests assert these behaviors for sync chat, sync generate, and async chat. ChangesException Handling and Validation
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-ollama/tests/test_exceptions.py (1)
11-98: ⚡ Quick winAdd regression tests for streaming failures after
_requestreturnsThese tests validate call-time exceptions, but not stream-consumption exceptions. Please add sync/async
stream=Truecases where the iterator raises mid-stream, then assert span is ended and markedStatusCode.ERRORwith an exception event.🤖 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-ollama/tests/test_exceptions.py` around lines 11 - 98, Add two new tests mirroring the existing ones but using stream=True to exercise streaming failures: for sync, patch OllamaClient._request to return an iterator that yields one item then raises an Exception("stream error"), call ollama.chat(..., stream=True) and iterate to trigger the error; for async, patch OllamaAsyncClient._request to return an async iterator that yields then raises and await iteration from ollama.AsyncClient().chat(..., stream=True). In both tests instrument with OllamaInstrumentor (instrument/uninstrument as in the other tests) and after the exception assert exactly one finished span exists, that span.status.status_code == StatusCode.ERROR, span.status.description contains the error message, and that span.events contains an event with name "exception" to confirm span.end() and exception recording.
🤖 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.
Inline comments:
In
`@packages/opentelemetry-instrumentation-ollama/opentelemetry/instrumentation/ollama/__init__.py`:
- Around line 312-318: The try/except around the immediate call only handles
call-time exceptions and misses errors raised while consuming streaming
iterators; update the streaming-paths so that any exception raised during
iteration in _accumulate_streaming_response and _aaccumulate_streaming_response
is caught, call span.set_status(Status(StatusCode.ERROR, str(e))) and
span.record_exception(e), ensure span.end() is always called before re-raising
the exception, and likewise wrap the generator/async-generator returned for
stream=True so iteration failures trigger the same error recording and span
closure (refer to span, _accumulate_streaming_response,
_aaccumulate_streaming_response and the stream-returning wrapper functions).
---
Nitpick comments:
In `@packages/opentelemetry-instrumentation-ollama/tests/test_exceptions.py`:
- Around line 11-98: Add two new tests mirroring the existing ones but using
stream=True to exercise streaming failures: for sync, patch
OllamaClient._request to return an iterator that yields one item then raises an
Exception("stream error"), call ollama.chat(..., stream=True) and iterate to
trigger the error; for async, patch OllamaAsyncClient._request to return an
async iterator that yields then raises and await iteration from
ollama.AsyncClient().chat(..., stream=True). In both tests instrument with
OllamaInstrumentor (instrument/uninstrument as in the other tests) and after the
exception assert exactly one finished span exists, that span.status.status_code
== StatusCode.ERROR, span.status.description contains the error message, and
that span.events contains an event with name "exception" to confirm span.end()
and exception recording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fe7e748-d544-46e8-8892-de2364c58201
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-ollama/opentelemetry/instrumentation/ollama/__init__.pypackages/opentelemetry-instrumentation-ollama/tests/test_exceptions.py
Streaming errors raised while consuming _accumulate_streaming_response or _aaccumulate_streaming_response were not caught, leaving spans open without an error status. Wrap both generator loops in try/except/finally so that: - iteration errors set span status to ERROR and record the exception - span.end() is always called via finally (success and error paths)
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.
Problem
Fixes #412
When
Client._requestorAsyncClient._requestraises an exception(e.g. HTTP error, connection refused, model not found), the ollama
instrumentation:
span.statustoERRORspan.record_exception()span.end()The span leaked open and errors were invisible in traces.
Fix
Wrapped the
wrapped(*args, **kwargs)call in both_wrapand_awrapwith try/except: