-
Notifications
You must be signed in to change notification settings - Fork 172
Ensure model wrapper gracefully handles closing the span #2552
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?
Ensure model wrapper gracefully handles closing the span #2552
Conversation
|
Hi! Thanks for the PR. I'm trying to understand the specific issue this is meant to address. Could you please share:
This would help us evaluate whether the change is necessary and correct. Thanks! Looking at the OpenTelemetry Python source, @_agnosticcontextmanager
def start_as_current_span(self, ..., end_on_exit=True):
span = self.start_span(...)
with trace_api.use_span(span, end_on_exit=end_on_exit, ...):
yield spanComparing the two approaches, they appear to be functionally equivalent:
While the structure differs, the end result is the same—both attach/detach context, end the span, and handle exceptions. |
| # Manually attach context (instead of using use_span context manager) | ||
| token = context_api.attach(trace_api.set_span_in_context(span)) | ||
|
|
||
| try: |
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.
Bug: Span never ended if context attachment fails
The span is created on line 495, but the try block doesn't start until line 509. The context_api.attach() call on line 507 sits between span creation and the try block. If context_api.attach() or trace_api.set_span_in_context() throws an exception, the span will never be ended because the finally block containing span.end() won't execute. The try block should wrap both the context attachment and the span to ensure span.end() is always called.
| except Exception as e: | ||
| span.set_status(trace_api.StatusCode.ERROR, str(e)) | ||
| span.record_exception(e) | ||
| raise |
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.
Bug: CancelledError not caught, span status remains OK
The exception handler catches only Exception, but asyncio.CancelledError inherits from BaseException in Python 3.8+. When an async streaming operation is cancelled, CancelledError won't be caught, the span status remains OK (set on line 510), and no exception is recorded. Other similar instrumentations in this repository (like autogen-agentchat) explicitly catch BaseException and handle GeneratorExit separately to properly record cancellation errors while allowing graceful generator cleanup.
Note
Manually manage the span in
arun_streamto ensure proper context handling, error recording, and guaranteed span closure for async streaming._model_wrapper.arun_stream):start_as_current_spanwith manualstart_spanand explicit context attach/detach.end()the span (handles cancellation/mismatched context).Written by Cursor Bugbot for commit 86b0f30. This will update automatically on new commits. Configure here.