-
Notifications
You must be signed in to change notification settings - Fork 570
fix(openai-agents): Avoid double span exit on exception #5174
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
Changes from 24 commits
9f2c047
4ca61e6
2c0edd5
cea080b
f9521de
5a70ca0
baa1b59
e6e40b1
cb23da0
bc982ed
a8fb881
557fc90
dd3063b
e5d5c52
e738f3d
64c2cfa
cea38a2
90f5dba
59732ed
e9e9e3a
4580edc
0477a4b
ef3ddc6
470bbbb
ea91e54
b3e5d01
5d9e0d0
c7a8a44
65a230f
ba09279
1acb4d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,25 @@ | ||
| from functools import wraps | ||
|
|
||
| from sentry_sdk.integrations import DidNotEnable | ||
| from ..spans import invoke_agent_span, update_invoke_agent_span, handoff_span | ||
| from sentry_sdk.tracing_utils import set_span_errored | ||
| from ..spans import ( | ||
| invoke_agent_span, | ||
| update_invoke_agent_span, | ||
| end_invoke_agent_span, | ||
| handoff_span, | ||
| ) | ||
| from ..utils import _capture_exception, _record_exception_on_span, _SingleTurnException | ||
|
|
||
| from typing import TYPE_CHECKING | ||
|
|
||
| if TYPE_CHECKING: | ||
| from typing import Any, Optional | ||
|
|
||
| from sentry_sdk.tracing import Span | ||
|
|
||
| try: | ||
| import agents | ||
| from agents.exceptions import AgentsException | ||
| except ImportError: | ||
| raise DidNotEnable("OpenAI Agents not installed") | ||
|
|
||
|
|
@@ -27,13 +37,15 @@ def _patch_agent_run(): | |
| original_execute_final_output = agents._run_impl.RunImpl.execute_final_output | ||
|
|
||
| def _start_invoke_agent_span(context_wrapper, agent, kwargs): | ||
| # type: (agents.RunContextWrapper, agents.Agent, dict[str, Any]) -> None | ||
| # type: (agents.RunContextWrapper, agents.Agent, dict[str, Any]) -> Span | ||
| """Start an agent invocation span""" | ||
| # Store the agent on the context wrapper so we can access it later | ||
| context_wrapper._sentry_current_agent = agent | ||
| span = invoke_agent_span(context_wrapper, agent, kwargs) | ||
| context_wrapper._sentry_agent_span = span | ||
|
|
||
| return span | ||
|
|
||
| def _end_invoke_agent_span(context_wrapper, agent, output=None): | ||
| # type: (agents.RunContextWrapper, agents.Agent, Optional[Any]) -> None | ||
| """End the agent invocation span""" | ||
|
|
@@ -65,18 +77,33 @@ async def patched_run_single_turn(cls, *args, **kwargs): | |
| context_wrapper = kwargs.get("context_wrapper") | ||
| should_run_agent_start_hooks = kwargs.get("should_run_agent_start_hooks") | ||
|
|
||
| span = getattr(context_wrapper, "_sentry_agent_span", None) | ||
| # Start agent span when agent starts (but only once per agent) | ||
| if should_run_agent_start_hooks and agent and context_wrapper: | ||
| # End any existing span for a different agent | ||
| if _has_active_agent_span(context_wrapper): | ||
| current_agent = _get_current_agent(context_wrapper) | ||
| if current_agent and current_agent != agent: | ||
| _end_invoke_agent_span(context_wrapper, current_agent) | ||
| end_invoke_agent_span(context_wrapper, current_agent) | ||
|
|
||
| _start_invoke_agent_span(context_wrapper, agent, kwargs) | ||
| span = _start_invoke_agent_span(context_wrapper, agent, kwargs) | ||
|
|
||
| # Call original method with all the correct parameters | ||
| result = await original_run_single_turn(*args, **kwargs) | ||
| try: | ||
| result = await original_run_single_turn(*args, **kwargs) | ||
| except AgentsException: | ||
| # AgentsException is caught on AgentRunner.run(). | ||
| # Exceptions are captured and agent invocation spans are explicitly finished | ||
| # as long as only AgentRunner.run() invokes AgentRunner._run_single_turn(). | ||
| raise | ||
| except Exception as exc: | ||
| _capture_exception(exc) | ||
|
|
||
| if span is not None and span.timestamp is None: | ||
| _record_exception_on_span(span, exc) | ||
| end_invoke_agent_span(context_wrapper, agent) | ||
|
|
||
| raise _SingleTurnException(exc) | ||
|
||
|
|
||
| return result | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,9 +1,15 @@ | ||||
| from functools import wraps | ||||
|
|
||||
| import sentry_sdk | ||||
| from sentry_sdk.integrations import DidNotEnable | ||||
|
|
||||
| from ..spans import agent_workflow_span | ||||
| from ..utils import _capture_exception | ||||
| from ..spans import agent_workflow_span, end_invoke_agent_span | ||||
alexander-alderman-webb marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
| from ..utils import _capture_exception, _record_exception_on_span, _SingleTurnException | ||||
|
|
||||
| try: | ||||
| from agents.exceptions import AgentsException | ||||
| except ImportError: | ||||
| raise DidNotEnable("OpenAI Agents not installed") | ||||
|
|
||||
| from typing import TYPE_CHECKING | ||||
|
|
||||
|
|
@@ -28,18 +34,36 @@ async def wrapper(*args, **kwargs): | |||
| with sentry_sdk.isolation_scope(): | ||||
| agent = args[0] | ||||
| with agent_workflow_span(agent): | ||||
| result = None | ||||
| try: | ||||
| result = await original_func(*args, **kwargs) | ||||
| return result | ||||
| except Exception as exc: | ||||
| run_result = await original_func(*args, **kwargs) | ||||
| except AgentsException as exc: | ||||
| _capture_exception(exc) | ||||
|
|
||||
| # It could be that there is a "invoke agent" span still open | ||||
| current_span = sentry_sdk.get_current_span() | ||||
| if current_span is not None and current_span.timestamp is None: | ||||
| current_span.__exit__(None, None, None) | ||||
| context_wrapper = getattr(exc.run_data, "context_wrapper", None) | ||||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
| if context_wrapper is not None: | ||||
| invoke_agent_span = getattr( | ||||
| context_wrapper, "_sentry_agent_span", None | ||||
| ) | ||||
|
|
||||
| if ( | ||||
| invoke_agent_span is not None | ||||
| and invoke_agent_span.timestamp is None | ||||
| ): | ||||
| _record_exception_on_span(invoke_agent_span, exc) | ||||
| end_invoke_agent_span(context_wrapper, agent) | ||||
|
|
||||
| raise exc from None | ||||
| except _SingleTurnException as exc: | ||||
| # Handled in _run_single_turn() patch. | ||||
| raise exc.original from None | ||||
| except Exception as exc: | ||||
| # Invoke agent span is not finished in this case. | ||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could add a You also can't guarantee that the SDK does not crash if you try to exit a span you obtain with Much of |
||||
| # This is much less likely to occur than other cases because | ||||
| # AgentRunner.run() is "just" a while loop around _run_single_turn. | ||||
| _capture_exception(exc) | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's hard for me to follow the flow, but my recommendation would be this:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry I meant that we should only have this one and not the one in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason there are special cases for exceptions, and for handling exceptions at different levels is that
Regarding having a finally block, I did not go with that approach for different reasons in In In
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
in that case keeping the span management in the except block is fine. But I still think the Regarding handling missing state on our end, we should simply write logic with presence checks so we don't rely on the actual underlying implementation of the lib so much. Currently, much of the design is very brittle to internals changing.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the span context manager
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've moved
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also realized we don't need this |
||||
| raise exc from None | ||||
|
|
||||
| end_invoke_agent_span(run_result.context_wrapper, agent) | ||||
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. duplicate of #5174 (comment) |
||||
| return run_result | ||||
|
|
||||
| return wrapper | ||||
Uh oh!
There was an error while loading. Please reload this page.