-
Notifications
You must be signed in to change notification settings - Fork 143
feat: Updated Agno attributes #2262
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: main
Are you sure you want to change the base?
feat: Updated Agno attributes #2262
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
...ation/openinference-instrumentation-agno/src/openinference/instrumentation/agno/_wrappers.py
Outdated
Show resolved
Hide resolved
...ation/openinference-instrumentation-agno/src/openinference/instrumentation/agno/_wrappers.py
Outdated
Show resolved
Hide resolved
...ation/openinference-instrumentation-agno/src/openinference/instrumentation/agno/_wrappers.py
Outdated
Show resolved
Hide resolved
I have read the CLA Document and I hereby sign the CLA |
...ation/openinference-instrumentation-agno/src/openinference/instrumentation/agno/_wrappers.py
Outdated
Show resolved
Hide resolved
...ation/openinference-instrumentation-agno/src/openinference/instrumentation/agno/_wrappers.py
Outdated
Show resolved
Hide resolved
...ation/openinference-instrumentation-agno/src/openinference/instrumentation/agno/_wrappers.py
Outdated
Show resolved
Hide resolved
...ation/openinference-instrumentation-agno/src/openinference/instrumentation/agno/_wrappers.py
Show resolved
Hide resolved
run_response = agent.get_last_run_output(session_id=session_id) | ||
if "session" in arguments and arguments.get("session") and len(arguments.get("session").runs) > 0: | ||
for run in arguments.get("session").runs: | ||
if run.content: |
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.
But if the session has many runs, this won't work? You'll have to get the last run in the session.
if run_response is not None: | ||
span.set_attribute(OUTPUT_VALUE, run_response.to_json()) | ||
span.set_attribute(OUTPUT_MIME_TYPE, JSON) | ||
else: |
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.
Is this Else necessary?
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.
Its a last resort fallback. Users that have caching or a DB enabled, might still get a response here.
for i, message in enumerate(messages): | ||
role, content = message.role, message.get_content_string() | ||
if content: | ||
if role in ["system", "user", "assistant"]: |
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.
What about tool results messages?
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.
This is on input. We map the tool results in the output messages
else: | ||
return str(output) | ||
|
||
if hasattr(output, 'role') and hasattr(output, 'content') and hasattr(output, 'tool_calls'): |
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.
Some output would have only content, and some only tool_calls. Should it not be or
?
"role": getattr(output, 'role', None), | ||
"content": getattr(output, 'content', None), | ||
"tool_calls": getattr(output, 'tool_calls', []), | ||
"event": getattr(output, 'event', None), |
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.
I don't think we should include this.
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.
removed
|
||
messages.append(result_dict) | ||
|
||
return {"messages": messages} |
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.
Should it return like this? Should it not just be a single string as the output?
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.
We do a json dumps with the response dict right?
Note
Upgrade to Agno v2 and update wrappers to extract cleaner inputs/outputs, aggregate streamed results, and record LLM token/cache metrics with refined message attributes.
agno
to>=2.1.2
(tests pin==2.1.2
).input.value
from user message content viaRunOutput
/RunMessages
(_get_user_message_content
).output.value
fromRunOutput.content
(_extract_run_response_output
).session.runs
oragent.get_last_run_output
; ensureoutput.mime_type=application/json
and OK status.messages
only; limit roles tosystem|user|assistant
inllm.input_messages
.llm.output_messages
; aggregate streamed chunks into a single message (_parse_model_output_stream
).llm.token_count.prompt
,llm.token_count.completion
, and cacheread/write
tokens on sync/async (new span attributes added)._parse_model_output
,_parse_model_output_stream
,_extract_run_response_output
; define token/cache span attribute constants.Written by Cursor Bugbot for commit fc65734. This will update automatically on new commits. Configure here.