chore: refactor use_span to be close automatically #1293
Conversation
poshinchen
had a problem deploying
to
auto-approve
GitHub Actions
Failure
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
poshinchen
had a problem deploying
to
auto-approve
GitHub Actions
Failure
poshinchen
had a problem deploying
to
auto-approve
GitHub Actions
Failure
poshinchen
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
zastrowm
left a comment
zastrowm
left a comment
There was a problem hiding this comment.
Agree with the intent of the changes, but have some nit callouts about the docs/code
| ) -> None: | ||
| """End an event loop cycle span with results. | ||
| Note: When using with trace_api.use_span(end_on_exit=True), the span will be automatically |
There was a problem hiding this comment.
We're forcing callers to close this now, right? If so, document that instead of When using with trace_api.use_span(end_on_exit=True), as that implies they can do this, but we're saying You must do this or close it explicitly
There was a problem hiding this comment.
OTEL will end the span automatically when exiting the function (regardless of success or exception being thrown). Previously we ended the span and recorded the exception by ourselves in our tracer setup in the tracer.end_xxx method. This is not ideal as OTEL can record the exception by itself.
| message: The message response from this cycle. | ||
| tool_result_message: Optional tool result message if a tool was called. | ||
| error: Optional exception if the cycle failed. | ||
| error: Optional exception if the cycle failed (not used when end_on_exit=True). |
There was a problem hiding this comment.
I'll remove this! It should be always True.
| self._add_event(span, "gen_ai.choice", event_attributes=event_attributes) | ||
| self._end_span(span, attributes, error) | ||
|
|
||
| # Note: self._end_span() is commented out because when using trace_api.use_span(end_on_exit=True), |
There was a problem hiding this comment.
Callers have no control over this; we can leave an explaining comment saying "We don't use self._end_span() because callers are responsible via closing"
But don't leave this commented out, just remove it
| }, | ||
| ) | ||
| mock_span.set_status.assert_called_once_with(StatusCode.OK) | ||
| mock_span.end.assert_called_once() |
There was a problem hiding this comment.
Should we be asserting that it was closed correctly?
There was a problem hiding this comment.
it is not closed in our tracer anymore, but I can add validation in other places.
2 participants
Description
Agentcore with strands sometimes see the warning message of
Tried calling _add_event on an ended spanTried calling _add_event on an ended span.In our current code base, we control the lifecycle of the spans, which sometimes could go wrong. Therefore, this PR makes the change to let OTEL handle the lifecycle in the use_span (end_on_exit=True).
Related Issues
Issue on Agentcore
Documentation PR
N/A
Type of Change
Bug fix
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.