-
Notifications
You must be signed in to change notification settings - Fork 571
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
base: master
Are you sure you want to change the base?
Changes from all 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
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) | ||
|
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 created Otherwise, exceptions raised in |
||
|
|
||
| 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) | ||
|
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. Bug: AttributeError when accessing exc.run_data without checking existenceThe code accesses
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. In practice it's always set as |
||
| 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) | ||
| raise exc from None | ||
|
|
||
| end_invoke_agent_span(run_result.context_wrapper, agent) | ||
|
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. Bug: 🔍 Detailed AnalysisLine 66 in 💡 Suggested FixReplace direct access 🤖 Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews.
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.