-
Notifications
You must be signed in to change notification settings - Fork 2
Fix/SAO-259 operation trace ownership #187
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 all commits
3bbcf19
0eaebdb
bdaadcb
c3d7788
72437a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,7 +41,7 @@ class SplunkAOTracingProcessor(TracingProcessor): | |
| Stores Node objects keyed by their OpenAI span_id or trace_id (for root). | ||
| """ | ||
|
|
||
| def __init__(self, splunk_ao_logger: SplunkAOLogger | None = None, flush_on_trace_end: bool = True): | ||
| def __init__(self, splunk_ao_logger: SplunkAOLogger | None = None, flush_on_trace_end: bool = False): | ||
| """ | ||
| OpenAI Agents TracingProcessor for logging traces to Splunk AO. | ||
|
|
||
|
|
@@ -58,6 +58,7 @@ def __init__(self, splunk_ao_logger: SplunkAOLogger | None = None, flush_on_trac | |
| self._last_output: Any = None | ||
| self._last_status_code: int | None = None | ||
| self._first_input: Any = None | ||
| self._owned_trace: Any = None | ||
|
|
||
| def on_trace_start(self, trace: Trace) -> None: | ||
| """Called when an OpenAI Agent trace starts.""" | ||
|
|
@@ -82,17 +83,19 @@ def on_trace_end(self, trace: Trace) -> None: | |
|
|
||
| node.span_params["duration_ns"] = convert_time_delta_to_ns(_get_timestamp() - node.span_params["start_time"]) | ||
|
|
||
| # Log the trace to Splunk AO (this includes concluding the trace) | ||
| self._commit_trace(trace) | ||
| self._nodes = {} | ||
|
|
||
| self._last_output = None | ||
| self._last_status_code = None | ||
| self._first_input = None | ||
|
|
||
| # Optionally flush the log batch | ||
| if self._flush_on_trace_end: | ||
| self._splunk_ao_logger.flush() | ||
| try: | ||
| self._commit_trace(trace) | ||
| if self._flush_on_trace_end: | ||
| self._splunk_ao_logger.flush() | ||
| except Exception: | ||
| self._conclude_current_trace_on_failure() | ||
| _logger.warning("Failed to commit OpenAI Agents telemetry", exc_info=True) | ||
| finally: | ||
| self._nodes = {} | ||
| self._last_output = None | ||
| self._last_status_code = None | ||
| self._first_input = None | ||
| self._owned_trace = None | ||
|
|
||
| def _commit_trace(self, trace: Trace) -> None: | ||
| if not self._nodes: | ||
|
|
@@ -106,6 +109,20 @@ def _commit_trace(self, trace: Trace) -> None: | |
| _logger.warning(f"Root node {trace.trace_id} not found") | ||
| self._splunk_ao_logger.conclude(output=self._last_output, status_code=self._last_status_code) | ||
|
|
||
| def _conclude_current_trace_on_failure(self) -> None: | ||
| if self._owned_trace is None: | ||
| return | ||
|
|
||
| current_parent = self._splunk_ao_logger.current_parent() | ||
| if current_parent is None: | ||
| return | ||
|
|
||
| root = current_parent | ||
| while root._parent is not None: | ||
| root = root._parent | ||
| if root is self._owned_trace: | ||
| self._splunk_ao_logger.conclude(output="", status_code=500, conclude_all=True) | ||
|
Comment on lines
+112
to
+124
Collaborator
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. 🟡 minor (testing): 🤖 Generated by the Astra agent
Collaborator
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. Added coverage. |
||
|
|
||
| def _log_node_tree(self, node: Node, first_node: bool = False) -> None: | ||
| """ | ||
| Log a node and its children recursively. | ||
|
|
@@ -129,7 +146,7 @@ def _log_node_tree(self, node: Node, first_node: bool = False) -> None: | |
| if metadata is not None: | ||
| metadata = convert_to_string_dict(metadata) | ||
| if first_node: | ||
| self._splunk_ao_logger.add_trace( | ||
| self._owned_trace = self._splunk_ao_logger.add_trace( | ||
| input=self._first_input or "Agent Workflow", | ||
| output=self._last_output, | ||
| duration_ns=node.span_params.get("duration_ns"), | ||
|
|
||
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.
🟡 minor (design):
commit()previously let exceptions fromstart_trace/log_node_tree/conclude/flushpropagate (only thefinallycleaned up). This newexcept Exception:block swallows all of them and downgrades to a warning. Silently dropping a partially-built trace is defensible for a telemetry callback, but it's a real change in error-propagation semantics for langchain/crewai integrations. Please confirm this swallowing is intended (and ideally note it) — if a framework relied onon_chain_endsurfacing handler errors, it will now go quiet. Same pattern was added inbase_async_handler.async_commitandopenai_agents/handler.on_trace_end.🤖 Generated by the Astra agent
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.
Confirmed intentional. These are telemetry callback failures and must not fail the instrumented LangChain/CrewAI operation. The handler logs the failure, cleans up only a trace it owns, and preserves any caller-owned trace.