kaxil commented on code in PR #72786:
URL: https://github.com/apache/airflow/pull/72786#discussion_r4049261644
##########
providers/common/ai/docs/toolsets.rst:
##########
@@ -1237,6 +1237,57 @@ derives one from a method name when there is no
docstring.
name and the description are the whole of what the model knows about the
agent.
+Metrics
+^^^^^^^
+
+Every call -- through a lone toolset or a ``FailoverManagedAgentToolset``
+alike -- emits ``managed_agent.invoked``, tagged by ``tool`` and ``platform``,
+whether it succeeds or fails. It is emitted before ``invoke()`` runs, from the
+shared ``call_tool()`` path that every toolset inherits, so a lone toolset
+gets a per-tool call count it would otherwise have no metric for at all, and a
+group gets exactly one increment per call regardless of how many members were
+tried -- an attempt, not an answer, so it keeps moving even during a total
+outage that leaves nothing to divide by otherwise.
+
+For a group, the ``platform`` tag on ``managed_agent.invoked`` is always the
+literal string ``"failover"``, not the cloud that actually answered --
+``agent_ref`` on a group describes the group's own identity, not whichever
+member ends up serving the call. Summing ``managed_agent.invoked`` by
+``platform`` therefore mixes that ``failover`` bucket in with real platform
+names from lone toolsets; filter by ``tool`` instead when a group and its
+members share a dashboard.
+
+``platform`` can also show up as the literal string ``"unknown"``, but only
+when a subclass's ``agent_ref`` returns a dict with no ``platform`` key: the
+tag degrades rather than failing the call over a missing label. It is not the
Review Comment:
This sentence is unqualified but only holds for `managed_agent.invoked`.
`managed_agent.served` and `managed_agent.failover` read the `{}` that
`_resolve_agent_ref` returns through `.get("platform", "unknown")`
(managed_agent.py:405-406 and :419), so on those two `unknown` *is* exactly the
bucket for an identity that failed to resolve, and newly so: before this PR a
raising member `agent_ref` killed `invoke()` before either counter was reached.
The cost is an inverted diagnosis, since an operator watching
`served{platform="unknown"}` climb is pointed at a subclass authoring bug when
the real cause is a broken member connection that the task log already names.
##########
shared/observability/src/airflow_shared/observability/metrics/metrics_template.yaml:
##########
@@ -415,6 +415,20 @@ metrics:
legacy_name: "-"
name_variables: ["tool", "platform", "role", "position"]
+ - name: "managed_agent.invoked"
+ description: "Number of managed agent invocations attempted through a
+ ``BaseManagedAgentToolset`` tool call, emitted once per attempt whether it
+ succeeds or fails, and whether the toolset stands alone or is a member of a
+ ``FailoverManagedAgentToolset`` group. Metric with tool and platform
tagging;
+ platform is the literal string ``unknown`` when the toolset's identity
fails
Review Comment:
Three clauses here describe the design the last commit replaced. `call_tool`
now reads `self.agent_ref` unguarded, so an identity that *fails to resolve*
aborts before the `Stats.incr` and emits nothing at all, which
`test_a_toolsets_own_broken_agent_ref_surfaces` pins with
`pytest.raises(RuntimeError)`. So `unknown` is not that bucket, the call does
not proceed after a warning, and a *member* of a group never emits this counter
either, since the group calls `member.invoke()` and
`test_group_call_tool_emits_invoked_once_regardless_of_failover` asserts
exactly one `invoked`. `toolsets.rst` in this same PR states it correctly,
which is the tell. This one matters more than a normal doc nit because the
description renders verbatim into the public metrics reference and
`check_metrics_synced_with_the_registry.py` validates only name and type, never
`description`, so nothing catches it later. The accurate version: `unknown`
when the returned dict has no `platform` key, and a group emits
once under its own tool name with `platform="failover"`.
##########
providers/common/ai/tests/unit/common/ai/toolsets/test_managed_agent.py:
##########
@@ -435,3 +479,154 @@ async def test_model_retry_is_not_a_failover(self,
mock_stats):
with pytest.raises(ModelRetry):
await group.invoke("q")
mock_stats.incr.assert_not_called()
+
+
+class TestSafeAgentRef:
Review Comment:
`_safe_agent_ref` became `_resolve_agent_ref`, and this class name is the
last place the old one survives, so grepping the new symbol misses its own
tests. `TestResolveAgentRef` would also match how the sibling classes in this
file are named.
##########
providers/common/ai/src/airflow/providers/common/ai/toolsets/managed_agent.py:
##########
@@ -320,7 +386,7 @@ async def invoke(self, prompt: str) -> Any:
except self._failover_on:
if position == last:
raise
- standby = self._members[position + 1].agent_ref
+ standby = _resolve_agent_ref(self._members[position + 1])
Review Comment:
Now that this can hand back `{}`, the warning three lines down renders as
`Managed agent None on None failed; failing over to None`, and the `served by
standby` log at :412 has the same shape. The `Stats` tags five lines below
default to `"unknown"` and the `agent_ref` join at :372 defaults to `"?"`, so
these three reads look like the ones that got missed rather than a deliberate
choice. It lands on the one path this PR exists to make survivable, and
toolsets.rst:1268-1272 sends the operator to that warning to find out which
member is broken. Threading `position` into the message would help too, since
`type(toolset).__name__` cannot separate the two members of the
same-image-on-two-clouds setup the docs recommend.
##########
providers/common/ai/src/airflow/providers/common/ai/toolsets/managed_agent.py:
##########
@@ -234,6 +283,14 @@ async def call_tool(
) -> Any:
ref = self.agent_ref
log.info("Consulting managed agent %s on %s", ref.get("name"),
ref.get("platform"))
+ # Emitted before invoke() runs, not after -- an attempt, not an answer,
+ # so a total outage still moves this counter and it stays the right
+ # per-tool denominator for managed_agent.failover even when nothing
+ # succeeds at all.
+ Stats.incr(
+ "managed_agent.invoked",
+ tags={"tool": self._tool_name, "platform": ref.get("platform",
"unknown")},
Review Comment:
The `"unknown"` default has no test behind it: both `ref.get("platform",
"MUTANT")` and a bare `ref["platform"]` leave all 65 green, so the one path
that actually produces `unknown` (a subclass whose `agent_ref` returns a dict
with no `platform` key) is unpinned. That is likely why the yaml, the rst and
the code ended up describing `unknown` three different ways, and `agent_ref` is
an `@abstractmethod` whose "must contain platform and name" contract is
docstring-only, so this default is the real guard. `FakeManagedAgentToolset`
already takes a `platform` kwarg, so a case returning `{"name":
"specialist-1"}` and asserting `tags={"tool": "ask_specialist", "platform":
"unknown"}` is a couple of lines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
