fix: agno stream session id extraction #2270
Conversation
dosubot
bot
added
the
size:M
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| # Mock agent with get_last_run_output method that raises exception when session_id is None | ||
| mock_agent = Mock() | ||
|
|
||
| def mock_get_last_run_output(session_id: str = None) -> Mock: |
There was a problem hiding this comment.
The type hint for session_id should be Optional[str] since it has a default value of None. This would make the type annotation consistent with the parameter's actual behavior:
def mock_get_last_run_output(session_id: Optional[str] = None) -> Mock:This change would also require adding Optional to the imports from typing at the top of the file, though it's already imported in the diff.
| def mock_get_last_run_output(session_id: str = None) -> Mock: | |
| def mock_get_last_run_output(session_id: Optional[str] = None) -> Mock: |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
This comment came from an experimental review—please leave feedback if it was helpful/unhelpful. Learn more about experimental comments here.
| mock_agent.name = "Test Agent" | ||
|
|
||
| # Mock the internal _run_stream method with proper signature | ||
| def mock_run_stream(message: str, session_id: str = None, **kwargs: Any) -> Iterator[str]: |
There was a problem hiding this comment.
The type hint for session_id should be Optional[str] since it has a default value of None. This would make the type annotation consistent with the parameter's actual behavior:
def mock_run_stream(message: str, session_id: Optional[str] = None, **kwargs: Any) -> Iterator[str]:This change would align with Python's type hinting best practices and improve static type checking.
| def mock_run_stream(message: str, session_id: str = None, **kwargs: Any) -> Iterator[str]: | |
| def mock_run_stream(message: str, session_id: Optional[str] = None, **kwargs: Any) -> Iterator[str]: |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
This comment came from an experimental review—please leave feedback if it was helpful/unhelpful. Learn more about experimental comments here.
| setup_agno_instrumentation: Any, | ||
| ) -> None: | ||
| """Regression test: ensure session_id is properly extracted in streaming methods.""" | ||
| from unittest.mock import Mock |
There was a problem hiding this comment.
prefer vcr over mock - see LN69, and LN126 above for setup
|
Any shot this can be merged soon? currently breaking telemetry |
|
@JasonLovesDoggo Yep! we'll push this out today! |
|
@JasonLovesDoggo Sorry! spoke a bit too soon; this should be out by tomorrow! |
dosubot
bot
added
size:L
|
Hey @GeLi2001 any update on this one? Dirk mentioned it is solved in their PR. Let's get that one across the line for @JasonLovesDoggo since Agno's PR should fix it. Let me know if you have any issues. |
|
@JasonLovesDoggo should be fixed now via #2262! I just released a new version of the agno instrumentation as well. Thanks for your patience on this! |
|
closing this PR in favor of #2262 |
4 participants
resolves #2265
Note
Ensure session_id is reliably captured (from args or session object) during streaming runs and add regression coverage with VCR cassette.
'_extract_session_id'with fallback toarguments['session'].session_idfor Agno v2 compatibility.'_extract_session_id'in'_run_arguments','_RunWrapper.run_stream', and'_RunWrapper.arun_stream'to setsession.idand fetchagent.get_last_run_output(session_id=...).test_session_id_streaming_regressionvalidatingsession.idis captured forstream=Trueruns.tests/openinference/instrumentation/agno/fixtures/agent_run_stream.yamlfor streaming responses.Written by Cursor Bugbot for commit 7b44140. This will update automatically on new commits. Configure here.