diff --git a/src/google/adk/telemetry/_instrumentation.py b/src/google/adk/telemetry/_instrumentation.py index 325a63ca5a..d67c2fc5f0 100644 --- a/src/google/adk/telemetry/_instrumentation.py +++ b/src/google/adk/telemetry/_instrumentation.py @@ -125,7 +125,6 @@ def record_llm_response( def _record_agent_metrics( agent_name: str, elapsed_s: float, - events: object, caught_error: Exception | None, ) -> None: try: @@ -134,7 +133,6 @@ def _record_agent_metrics( elapsed_s, caught_error, ) - _metrics.record_agent_workflow_steps(agent_name, events) except Exception: # pylint: disable=broad-exception-caught logger.exception("Failed to record agent metrics for agent %s", agent_name) @@ -196,7 +194,6 @@ async def record_agent_invocation( _record_agent_metrics( agent.name, _metrics.get_elapsed_s(span, start_time), - getattr(getattr(ctx, "session", None), "events", []), caught_error, ) _flush_invoke_agent_metrics(tel_ctx, agent.name) diff --git a/src/google/adk/telemetry/_metrics.py b/src/google/adk/telemetry/_metrics.py index d8bd82dd4e..be5df9d494 100644 --- a/src/google/adk/telemetry/_metrics.py +++ b/src/google/adk/telemetry/_metrics.py @@ -27,7 +27,6 @@ from opentelemetry.semconv.attributes import error_attributes if TYPE_CHECKING: - from google.adk.events.event import Event from google.adk.models.llm_request import LlmRequest from google.adk.models.llm_response import LlmResponse from opentelemetry.trace import Span @@ -91,24 +90,6 @@ 81.92, ], ) -_agent_workflow_steps = meter.create_histogram( - "gen_ai.agent.workflow.steps", - unit="1", - description="Length of agentic workflow (# of events).", - explicit_bucket_boundaries_advisory=[ - 1, - 2, - 4, - 8, - 16, - 32, - 64, - 128, - 256, - 512, - 1024, - ], -) _client_operation_duration = ( gen_ai_metrics.create_gen_ai_client_operation_duration(meter) ) @@ -198,13 +179,6 @@ def record_invoke_agent_tool_calls(agent_name: str, count: int) -> None: _invoke_agent_tool_calls.record(count, attributes=attrs) -def record_agent_workflow_steps(agent_name: str, events: list[Event]): - """Records the number of steps in the agent workflow by counting the number of events.""" - attrs = {gen_ai_attributes.GEN_AI_AGENT_NAME: agent_name} - count = sum(1 for event in events if event.author == agent_name) - _agent_workflow_steps.record(count, attributes=attrs) - - def record_tool_execution_duration( tool_name: str, tool_type: str, diff --git a/src/google/adk/telemetry/node_tracing.py b/src/google/adk/telemetry/node_tracing.py index 6e5fac59eb..cbfa6e97b6 100644 --- a/src/google/adk/telemetry/node_tracing.py +++ b/src/google/adk/telemetry/node_tracing.py @@ -186,10 +186,6 @@ def _use_invoke_workflow_span( # The flag rides along the otel_context propagated to child nodes, so nested # workflows see it set. nested = bool(context_api.get_value(_ENTRYPOINT_WORKFLOW_KEY, otel_context)) - if not nested: - otel_context = context_api.set_value( - _ENTRYPOINT_WORKFLOW_KEY, True, otel_context - ) attributes: dict[str, AttributeValue] = { GEN_AI_OPERATION_NAME: "invoke_workflow", GEN_AI_CONVERSATION_ID: conversation_id, @@ -207,11 +203,14 @@ def _use_invoke_workflow_span( start_s = time.monotonic() workflow_span: Span | None = None try: - with tracer.start_as_current_span( - name=span_name, - attributes=attributes, - context=otel_context, - ) as span: + with ( + tracer.start_as_current_span( + name=span_name, + attributes=attributes, + context=otel_context, + ) as span, + _mark_nested_workflows(), + ): workflow_span = span yield span finally: @@ -221,3 +220,14 @@ def _use_invoke_workflow_span( nested=nested, error=sys.exc_info()[1], ) + + +@contextmanager +def _mark_nested_workflows() -> Iterator[None]: + token = context_api.attach( + context_api.set_value(_ENTRYPOINT_WORKFLOW_KEY, True) + ) + try: + yield + finally: + context_api.detach(token) diff --git a/tests/unittests/telemetry/functional_node_test_cases.py b/tests/unittests/telemetry/functional_node_test_cases.py index c38d9c2427..e46e2c411e 100644 --- a/tests/unittests/telemetry/functional_node_test_cases.py +++ b/tests/unittests/telemetry/functional_node_test_cases.py @@ -43,6 +43,7 @@ from .functional_test_helpers import GEN_AI_USER_MESSAGE_EVENT from .functional_test_helpers import LogDigest from .functional_test_helpers import MetricPoint +from .functional_test_helpers import NESTED_WORKFLOW_NAME from .functional_test_helpers import NODE_NAME from .functional_test_helpers import NODE_RESULT from .functional_test_helpers import NON_DETERMINISTIC @@ -262,12 +263,25 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": ( + PRESENT + ), + }, + ), + ], ), ], ), @@ -539,12 +553,25 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": ( + PRESENT + ), + }, + ), + ], ), ], ), @@ -750,12 +777,25 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": ( + PRESENT + ), + }, + ), + ], ), ], ), @@ -1038,12 +1078,25 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": ( + PRESENT + ), + }, + ), + ], ), ], ), @@ -1257,12 +1310,25 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": ( + PRESENT + ), + }, + ), + ], ), ], ), @@ -1494,12 +1560,25 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": ( + PRESENT + ), + }, + ), + ], ), ], ), @@ -1659,12 +1738,23 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": PRESENT, + }, + ), + ], ), ], ) @@ -1876,12 +1966,23 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": PRESENT, + }, + ), + ], ), ], ) @@ -2038,12 +2139,23 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": PRESENT, + }, + ), + ], ), ], ) @@ -2206,12 +2318,23 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": PRESENT, + }, + ), + ], ), ], ) @@ -2380,12 +2503,23 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": PRESENT, + }, + ), + ], ), ], ) @@ -2568,12 +2702,23 @@ ], ), SpanDigest( - name=f"invoke_node {NODE_NAME}", + name=f"invoke_workflow {NESTED_WORKFLOW_NAME}", attributes={ - "gen_ai.operation.name": "invoke_node", + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, "gen_ai.conversation.id": PRESENT, - "gcp.vertex.agent.associated_event_ids": PRESENT, }, + children=[ + SpanDigest( + name=f"invoke_node {NODE_NAME}", + attributes={ + "gen_ai.operation.name": "invoke_node", + "gen_ai.conversation.id": PRESENT, + "gcp.vertex.agent.associated_event_ids": PRESENT, + }, + ), + ], ), ], ) @@ -2597,9 +2742,6 @@ value=NON_DETERMINISTIC, ), }), - "gen_ai.agent.workflow.steps": frozenset({ - MetricPoint(attributes={"gen_ai.agent.name": AGENT_NAME}, value=3), - }), "gen_ai.client.operation.duration": frozenset({ MetricPoint( attributes={ @@ -2620,6 +2762,16 @@ }, value=NON_DETERMINISTIC, ), + # Nested workflow carries the `gen_ai.workflow.nested` dimension; the + # root workflow above omits it. + MetricPoint( + attributes={ + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, + }, + value=NON_DETERMINISTIC, + ), }), "gen_ai.invoke_agent.inference_calls": frozenset({ MetricPoint(attributes={"gen_ai.agent.name": AGENT_NAME}, value=2), @@ -2647,9 +2799,6 @@ value=NON_DETERMINISTIC, ), }), - "gen_ai.agent.workflow.steps": frozenset({ - MetricPoint(attributes={"gen_ai.agent.name": AGENT_NAME}, value=3), - }), "gen_ai.client.operation.duration": frozenset({ MetricPoint( attributes={ @@ -2670,6 +2819,16 @@ }, value=NON_DETERMINISTIC, ), + # Nested workflow carries the `gen_ai.workflow.nested` dimension; the + # root workflow above omits it. + MetricPoint( + attributes={ + "gen_ai.operation.name": "invoke_workflow", + "gen_ai.workflow.name": NESTED_WORKFLOW_NAME, + "gen_ai.workflow.nested": True, + }, + value=NON_DETERMINISTIC, + ), }), "gen_ai.invoke_agent.inference_calls": frozenset({ MetricPoint(attributes={"gen_ai.agent.name": AGENT_NAME}, value=2), diff --git a/tests/unittests/telemetry/functional_test_cases.py b/tests/unittests/telemetry/functional_test_cases.py index 0c05107c5c..afce3bc340 100644 --- a/tests/unittests/telemetry/functional_test_cases.py +++ b/tests/unittests/telemetry/functional_test_cases.py @@ -2291,9 +2291,6 @@ value=NON_DETERMINISTIC, ), }), - "gen_ai.agent.workflow.steps": frozenset({ - MetricPoint(attributes={"gen_ai.agent.name": AGENT_NAME}, value=3), - }), "gen_ai.client.operation.duration": frozenset({ MetricPoint( attributes={ @@ -2332,9 +2329,6 @@ value=NON_DETERMINISTIC, ), }), - "gen_ai.agent.workflow.steps": frozenset({ - MetricPoint(attributes={"gen_ai.agent.name": AGENT_NAME}, value=3), - }), "gen_ai.client.operation.duration": frozenset({ MetricPoint( attributes={ diff --git a/tests/unittests/telemetry/functional_test_helpers.py b/tests/unittests/telemetry/functional_test_helpers.py index 7ff8e9f1a9..819b59bc0e 100644 --- a/tests/unittests/telemetry/functional_test_helpers.py +++ b/tests/unittests/telemetry/functional_test_helpers.py @@ -314,11 +314,6 @@ class HistogramSpec(NamedTuple): attr="_tool_execution_duration", metric_name="gen_ai.execute_tool.duration", ), - HistogramSpec( - module=_metrics, - attr="_agent_workflow_steps", - metric_name="gen_ai.agent.workflow.steps", - ), HistogramSpec( module=_metrics, attr="_client_operation_duration", @@ -504,6 +499,10 @@ def install_telemetry( # The node scenario uses a workflow node whose output drives the agent's # input. The workflow itself wraps the same agent. WORKFLOW_NAME = "my_workflow" +# The root workflow invokes a nested workflow whose sole node produces the +# input for the agent. The nested workflow exercises the `gen_ai.workflow.nested` +# span attribute + metric dimension (only nested workflows carry it). +NESTED_WORKFLOW_NAME = "my_nested_workflow" NODE_NAME = "some_node" NODE_RESULT = "some result" NODE_USER_ID = "some_user" @@ -550,15 +549,21 @@ def build_test_runner(*, failing: bool = False) -> TestInMemoryRunner: def build_test_workflow(*, failing: bool = False) -> Workflow: - """Builds the canonical Workflow wrapping the agent + a trivial node.""" + """Builds the canonical Workflow: a nested workflow feeding the agent.""" test_agent = build_test_agent(failing=failing) async def some_node(ctx, node_input): return NODE_RESULT + # Trivial workflow to test o11y of nested workflows + nested_workflow = Workflow( + name=NESTED_WORKFLOW_NAME, + edges=[(START, some_node)], + ) + return Workflow( name=WORKFLOW_NAME, - edges=[(START, some_node, test_agent)], + edges=[(START, nested_workflow, test_agent)], ) diff --git a/tests/unittests/telemetry/test_instrumentation.py b/tests/unittests/telemetry/test_instrumentation.py index 41de19b500..fc339c4a22 100644 --- a/tests/unittests/telemetry/test_instrumentation.py +++ b/tests/unittests/telemetry/test_instrumentation.py @@ -17,10 +17,8 @@ import time from unittest import mock -from google.adk.telemetry import _instrumentation from google.adk.telemetry import _metrics from opentelemetry import trace -import pytest def test_get_elapsed_s_span_none(): @@ -82,36 +80,3 @@ def test_get_elapsed_s_span_non_int_end(): with mock.patch("time.monotonic", return_value=12.0): elapsed = _metrics.get_elapsed_s(mock_span, start_time) assert elapsed == 2.0 - - -@pytest.mark.asyncio -async def test_record_agent_invocation_tolerates_minimal_context(): - """Tolerates context-likes that lack session. - - Test doubles, partial migrations, and external embedders can pass an - InvocationContext-like object with a `session` that has no `events` - attribute. The telemetry path must not raise AttributeError on the - metrics call in those cases. - """ - agent = mock.MagicMock() - agent.name = "test_agent" - # Bare object without `session`. - bare_ctx = object() - - with ( - mock.patch.object( - _instrumentation, "_record_agent_metrics" - ) as mock_record, - mock.patch.object(_instrumentation, "tracing") as mock_tracing, - ): - mock_tracing.tracer.start_as_current_span.return_value.__enter__.return_value = mock.MagicMock( - spec=trace.Span - ) - async with _instrumentation.record_agent_invocation(bare_ctx, agent): - pass - - mock_record.assert_called_once() - call_args = mock_record.call_args - # positional: (agent_name, elapsed_s, events, caught_error) - assert call_args.args[0] == "test_agent" - assert call_args.args[2] == [] # events default diff --git a/tests/unittests/telemetry/test_metrics.py b/tests/unittests/telemetry/test_metrics.py index 62d1d5c662..de2d976ce2 100644 --- a/tests/unittests/telemetry/test_metrics.py +++ b/tests/unittests/telemetry/test_metrics.py @@ -29,14 +29,12 @@ def _mock_meter_setup(monkeypatch): agent_duration_hist = mock.MagicMock(spec=metrics.Histogram) workflow_duration_hist = mock.MagicMock(spec=metrics.Histogram) tool_duration_hist = mock.MagicMock(spec=metrics.Histogram) - steps_hist = mock.MagicMock(spec=metrics.Histogram) client_duration_hist = mock.MagicMock(spec=metrics.Histogram) client_token_usage_hist = mock.MagicMock(spec=metrics.Histogram) agent_duration_hist.name = "agent_invocation_duration" workflow_duration_hist.name = "workflow_invocation_duration" tool_duration_hist.name = "tool_execution_duration" - steps_hist.name = "agent_workflow_steps" client_duration_hist.name = "client_operation_duration" client_token_usage_hist.name = "client_token_usage" @@ -47,8 +45,6 @@ def create_histogram_side_effect(name, **_kwargs): return workflow_duration_hist elif name == "gen_ai.execute_tool.duration": return tool_duration_hist - elif name == "gen_ai.agent.workflow.steps": - return steps_hist elif name == "gen_ai.client.operation.duration": return client_duration_hist elif name == "gen_ai.client.token.usage": @@ -66,7 +62,6 @@ def create_histogram_side_effect(name, **_kwargs): _metrics, "_workflow_invocation_duration", workflow_duration_hist ) monkeypatch.setattr(_metrics, "_tool_execution_duration", tool_duration_hist) - monkeypatch.setattr(_metrics, "_agent_workflow_steps", steps_hist) monkeypatch.setattr( _metrics, "_client_operation_duration", client_duration_hist ) @@ -77,7 +72,6 @@ def create_histogram_side_effect(name, **_kwargs): "agent_duration": agent_duration_hist, "workflow_duration": workflow_duration_hist, "tool_duration": tool_duration_hist, - "steps": steps_hist, "client_duration": client_duration_hist, "client_token_usage": client_token_usage_hist, } @@ -145,24 +139,6 @@ def test_record_workflow_invocation_duration_nested_with_error( assert kwargs["attributes"]["error.type"] == "ValueError" -def test_record_agent_workflow_steps(mock_meter_setup): - """Tests record_agent_workflow_steps records correctly.""" - _metrics.record_agent_workflow_steps( - "test_agent", - [ - mock.MagicMock(author="test_agent"), - mock.MagicMock(author="test_agent"), - mock.MagicMock(author="other_agent"), - ], - ) - steps_hist = mock_meter_setup["steps"] - steps_hist.record.assert_called_once() - args, kwargs = steps_hist.record.call_args - assert args[0] == 2 - want_attributes = {"gen_ai.agent.name": "test_agent"} - assert kwargs["attributes"] == want_attributes - - def test_record_tool_execution_duration(mock_meter_setup): """Tests record_tool_execution_duration records correctly.""" _metrics.record_tool_execution_duration(