[opentelemetry-instrumentation-genai-agno] Add instrumentation for Agno Team, Workflow methods. - #350
[opentelemetry-instrumentation-genai-agno] Add instrumentation for Agno Team, Workflow methods.#350DylanRussell wants to merge 31 commits into
opentelemetry-instrumentation-genai-agno] Add instrumentation for Agno Team, Workflow methods.#350Conversation
There was a problem hiding this comment.
Pull request overview
Adds additional Agno instrumentation coverage by patching Team, Workflow, Step, and Model response methods to emit GenAI spans via opentelemetry-util-genai, and updates package docs/tests accordingly.
Changes:
- Extend patching to
Team.run/arun,Workflow.run/arun,Step.execute/aexecute, andModel.response/aresponse. - Add unit tests covering Team + Model response instrumentation and basic Workflow/Step coverage.
- Document supported operations in
README.rstand add a changelog fragment.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| instrumentation/opentelemetry-instrumentation-genai-agno/src/opentelemetry/instrumentation/genai/agno/patch.py | Adds wrappers for Team/Workflow/Step/Model methods and model message extraction for inference spans. |
| instrumentation/opentelemetry-instrumentation-genai-agno/tests/test_agent.py | Adds tests for Team + Model response spans and basic Workflow/Step span assertions. |
| instrumentation/opentelemetry-instrumentation-genai-agno/README.rst | Documents newly supported operations. |
| instrumentation/opentelemetry-instrumentation-genai-agno/.changelog/350.added | Changelog entry for the added instrumentation coverage. |
Suppressed comments (2)
instrumentation/opentelemetry-instrumentation-genai-agno/src/opentelemetry/instrumentation/genai/agno/patch.py:108
- This PR instruments
Step.aexecute, but there is no corresponding async test coverage validating that span emission works for the async variant.
wrap_function_wrapper(
_AGNO_STEP_MODULE,
f"{_STEP_CLASS}.aexecute",
_step_aexecute(handler),
)
instrumentation/opentelemetry-instrumentation-genai-agno/tests/test_agent.py:309
- Skipping only on
fastapibeing absent may not cover other optional dependency/import failures inagno.workflow.step. It’s more robust to skip on importing the step module itself.
pytest.importorskip("fastapi")
from agno.workflow.step import Step # noqa: PLC0415
Pull request dashboard statusWaiting on the author · refreshed 2026-08-22 17:22 UTC Respond to 6 review items (e.g. link a commit, explain why not, ask a follow-up): Status above doesn't look right?
|
DylanRussell
changed the title
opentelemetry-instrumentation-genai-agno] Add instrumentation for Agno Team, Workflow, Step, and Model response methods.opentelemetry-instrumentation-genai-agno] Add instrumentation for Agno Team, Workflow, and Model response methods.
lmolkova
left a comment
lmolkova
left a comment
There was a problem hiding this comment.
LGTM, but the metric should be gen_ai.invoke_workflow.duration
# Conflicts: # instrumentation/opentelemetry-instrumentation-genai-agno/src/opentelemetry/instrumentation/genai/agno/patch.py
…guard conversation_id assignment
…low.duration and make upstream semconv filter optional
| return invocation | ||
|
|
||
|
|
||
| def _start_model_invocation( |
There was a problem hiding this comment.
do we need to report inference from agno? does it rely on openai / anthropic / google genai to make model calls? if so, why not rely on the corresponding instrumentations to do it?
Otherwise we end up with duplication if inference is recorded by both layers.
There was a problem hiding this comment.
They support a ton of models including ones we don't have instrumenation for and stuff like vLLM where you can bring up your own model (https://docs.agno.com/examples/models/overview) ..
If we instrument at the agno layer the invoke model span will look the exact same for every model call, versus it will look slightly different when generated at the model layer (because the model API interfaces differ, and the instrumentations differ slightly)..
It's easier to just install agno instrumentation, than install agno + X model provider instrumentations (probably not a big deal but still..)
ADK does the same thing FWIW..
Yeah duplication is a problem though.. I think probably the best way to handle it is for the agno instrumentation to NOT instrument the model call IF the models associated instrumentation is installed and the model has been monkey patched.. Before thinking about how to do that though, what do you think of my argument ?
There was a problem hiding this comment.
I think it makes sense to instrument layers that can't be instrumented otherwise.
Priority-wise - we deliver more value and coverage by instrumenting layers that uniquely belong to this library.
I propose to postpone inference instrumentation in agentic libs until we have a reasonable coverage for agentic layers and get back to it later when polishing onboarding experience or covering less popular inference providers.
There was a problem hiding this comment.
I'm going to instrument all of agno, I don't think we have to worry about prioritization and ranking value of particular parts of this library.. I think it makes sense to instrument this layer and address the problem now.. But I'll instrument the rest of the library first, I don't have a problem doing that either..
There was a problem hiding this comment.
Reverted the model inference instrumentation for now.. PTAL
There was a problem hiding this comment.
thanks! this is not about writing the code that does inference. Writing it is not a problem, but if we do it, we need to
- figure out suppression
- keep parity across all libs when we make changes
we already have this problem with openai and anthropic instrumented in langchain and in their own libs.
It's readonable to add inference for providers that don't and won't have their own instrumentation - this is what we just did in smolagents.
This is also now a part of the guidelines in this repo - https://github.com/open-telemetry/opentelemetry-python-genai/blob/main/CONTRIBUTING.md#which-operations-an-instrumentation-should-emit
|
Hi @DylanRussell — just a friendly reminder that this pull request is waiting on you. The dashboard status comment has the open items and is kept current.
|
DylanRussell
changed the title
opentelemetry-instrumentation-genai-agno] Add instrumentation for Agno Team, Workflow, and Model response methods.opentelemetry-instrumentation-genai-agno] Add instrumentation for Agno Team, Workflow methods.
| role=str(getattr(result, "role", "assistant")), | ||
| parts=[Text(content=output_str)], | ||
| finish_reason="stop", | ||
| finish_reason=str(getattr(result, "finish_reason", "stop")), |
There was a problem hiding this comment.
role and finish_reason don't exist on RunOutput, TeamRunOutput or WorkflowRunOutput (agno 2.8.6), so both getattrs always take the default. And if either field does appear later as None, str() turns it into the literal string "None":
@dataclass
class _Result:
content: str = "hi"
role: str | None = None
finish_reason: str | None = None
def test_none_role_becomes_string_none():
invocation = TelemetryHandler().workflow(name="wf")
_set_invocation_output(invocation, _Result(), capture_content=True)
invocation.stop()
msg = invocation.output_messages[0]
assert msg.role == "assistant"
assert msg.finish_reason == "stop"AssertionError: OutputMessage(role='None', parts=[Text(content='hi', type='text')], finish_reason='None')
Please use an existing field or fallback to stop on success / error on failure. Please make sure there is a test coverage
| ] | ||
| if hasattr(result, "session_id") and getattr(result, "session_id"): | ||
| if ( | ||
| isinstance(invocation, AgentInvocation) |
There was a problem hiding this comment.
WorkflowRunOutput has a session_id, but this isinstance gate silently drops it because WorkflowInvocation has no conversation_id:
def test_workflow_session_id(tracer_provider, logger_provider, meter_provider, span_exporter):
with instrument(AgnoInstrumentor(), tracer_provider=tracer_provider,
logger_provider=logger_provider, meter_provider=meter_provider,
content_capture="SPAN_ONLY"):
Workflow(name="wf", steps=[], session_id="session-1").run("hi")
span = span_exporter.get_finished_spans()[0]
assert span.attributes.get(GenAIAttributes.GEN_AI_CONVERSATION_ID) == "session-1"AssertionError: assert None == 'session-1'
{'gen_ai.operation.name': 'invoke_workflow', 'gen_ai.workflow.name': 'wf', ...}
Please add conversation_id to WorkflowInvocation and set it here
| workflow.run("test input") | ||
|
|
||
| spans = span_exporter.get_finished_spans() | ||
| assert any( |
There was a problem hiding this comment.
The two workflow tests don't assert much. patch.object(Workflow, "run", wraps=workflow.run) is a no-op - the bound method it wraps is already the instrumented one, and removing the patch produces the identical span. And assert any(... == "invoke_workflow") skips span count, span name, and gen_ai.workflow.name; the Team tests above assert all three.
Also missing across all four new tests: the error path (wrapped call raises -> span carries error.type and the exception re-raises unchanged).
| context=invocation._span_context, | ||
| ) | ||
|
|
||
| def record_workflow(self, invocation: GenAIInvocation) -> None: |
There was a problem hiding this comment.
Type this WorkflowInvocation, not GenAIInvocation. The base _get_metric_attributes() returns just metric_attributes - no gen_ai.operation.name, no gen_ai.workflow.name - so any other invocation type passed here records a silently wrong-shaped metric instead of failing.
| def create_workflow_duration_histogram(meter: Meter) -> Histogram: | ||
| name = getattr( | ||
| gen_ai_metrics, | ||
| "GEN_AI_INVOKE_WORKFLOW_DURATION", |
There was a problem hiding this comment.
nit: getattr(gen_ai_metrics, "GEN_AI_INVOKE_WORKFLOW_DURATION", ...) always takes the fallback - the constant doesn't exist in any released opentelemetry-semantic-conventions. It reads as forward-compat but is a hardcoded name in disguise, and if semconv-python ever defines that symbol with a different value the metric name flips silently. Please use the literal
| return invocation | ||
|
|
||
|
|
||
| def _start_model_invocation( |
There was a problem hiding this comment.
thanks! this is not about writing the code that does inference. Writing it is not a problem, but if we do it, we need to
- figure out suppression
- keep parity across all libs when we make changes
we already have this problem with openai and anthropic instrumented in langchain and in their own libs.
It's readonable to add inference for providers that don't and won't have their own instrumentation - this is what we just did in smolagents.
This is also now a part of the guidelines in this repo - https://github.com/open-telemetry/opentelemetry-python-genai/blob/main/CONTRIBUTING.md#which-operations-an-instrumentation-should-emit
Description
Add instrumentation for the following methods:
I'm not certain if it makes sense to model
teamsfunction call as ainvoke agentspan or if we might want to add a new sem conv span name for it.. But that is how both open inference and alibaba longsuite modeled it and it sort of makes sense. https://docs.agno.com/teams/overview#when-to-use-teams and https://docs.agno.com/workflows/overview#when-to-use-workflows describe the differences between workflows / agents / teams..Next steps:
Add run_stream / arun_stream / response_stream / aresponse_stream using SyncStreamWrapper and AsyncStreamWrapper.
Add Parallel.execute and Parallel.aexecute wrapping for workflow parallel steps.
Instrument
Step(awaiting on open-telemetry/semantic-conventions-genai#188)Instrument model invocations.. need to decide how to disable/prevent duplicate telemetry in this scenario..
Type of change
How has this been tested?
Unit tests
Checklist