Skip to content

Add auth submetrics#189

Open
maryna-shevchuk wants to merge 1 commit into
mainfrom
pr/ms/sub-metric-for-auth-success
Open

Add auth submetrics#189
maryna-shevchuk wants to merge 1 commit into
mainfrom
pr/ms/sub-metric-for-auth-success

Conversation

@maryna-shevchuk

Copy link
Copy Markdown
Collaborator
  1. Add auth submetrics. Submetrics only get computed when the agent actually calls an "auth" tool during the conversation:
  • auth_first_try_success: 1.0 if authentication overall succeeded AND every auth tool was called exactly once (no retries needed); 0.0 if it succeeded but only after retrying an auth tool.
  • auth_num_calls: average number of times each auth tool was called, but only counted when auth succeeded — tells you how many attempts it typically took to succeed.
  • auth_num_calls_on_failure: same average call count, but for cases where auth ultimately failed — tells you how many attempts the agent wasted before giving up.
  1. Add unit tests for the submetrics.

@JosephMarinier JosephMarinier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have small comments to simplify the code.

auth_success: bool,
) -> dict[str, MetricScore]:
"""Build sub-metrics for authentication tool call behaviour."""
auth_tool_names = {t.get("name") for t in agent_tools if t.get("tool_type") == "auth" and t.get("name")}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is required in the AgentTool Pydantic model, so we don't need to check for if t.get("name").

Suggested change
auth_tool_names = {t.get("name") for t in agent_tools if t.get("tool_type") == "auth" and t.get("name")}
auth_tool_names = {t["name"] for t in agent_tools if t.get("tool_type") == "auth"}

Comment on lines +104 to +110
auth_calls = [r for r in tool_responses if r.get("tool_name") in auth_tool_names]
if not auth_calls:
return {}

sub_metrics: dict[str, MetricScore] = {}

counts_per_tool = Counter(r.get("tool_name") for r in auth_calls)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detail, but instead of instantiating a list of auth_calls, we can pass a generator directly to the Counter. If there are no auth calls, the Counter is just as cheap as the list; if there are some auth calls, we save the allocation of the list. Just like the list, the Counter is falsy if empty, so if not counts_per_tool is equivalent to if not auth_calls.

Suggested change
auth_calls = [r for r in tool_responses if r.get("tool_name") in auth_tool_names]
if not auth_calls:
return {}
sub_metrics: dict[str, MetricScore] = {}
counts_per_tool = Counter(r.get("tool_name") for r in auth_calls)
counts_per_tool = Counter(r["tool_name"] for r in tool_responses if r.get("tool_name") in auth_tool_names)
sub_metrics: dict[str, MetricScore] = {}
if not counts_per_tool:
return sub_metrics

sub_metrics: dict[str, MetricScore] = {}

counts_per_tool = Counter(r.get("tool_name") for r in auth_calls)
avg_calls = sum(counts_per_tool.values()) / len(counts_per_tool) if counts_per_tool else 0.0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above if and early return already guarantee that there are some auth tools, so the if counts_per_tool is unnecessary.

Suggested change
avg_calls = sum(counts_per_tool.values()) / len(counts_per_tool) if counts_per_tool else 0.0
avg_calls = sum(counts_per_tool.values()) / len(counts_per_tool)

Comment on lines +121 to +132
sub_metrics["auth_num_calls"] = MetricScore(
name=f"{parent_name}.auth_num_calls",
score=round(avg_calls, 4),
normalized_score=None,
details={"calls_per_tool": counts_per_tool, "num_auth_tools": len(counts_per_tool)},
)
else:
sub_metrics["auth_num_calls_on_failure"] = MetricScore(
name=f"{parent_name}.auth_num_calls_on_failure",
score=round(avg_calls, 4),
normalized_score=None,
details={"calls_per_tool": counts_per_tool, "num_auth_tools": len(counts_per_tool)},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sharing a few thoughts on naming in case it resonates, but I'm also fine with the current naming.

  1. I think num_auth_calls flows more naturally than auth_num_calls. Our code has many instances of num_something_something metrics and variables, like num_tool_calls and num_assistant_turns, but no something_num_something.
  2. We could make calls_per_tool more specific, like num_auth_calls_per_tool (or auth_num_calls_per_tool). These are numbers, not the calls themselves, and they only include the auth calls. But maybe shorter is better. I'm fine either way.
Suggested change
sub_metrics["auth_num_calls"] = MetricScore(
name=f"{parent_name}.auth_num_calls",
score=round(avg_calls, 4),
normalized_score=None,
details={"calls_per_tool": counts_per_tool, "num_auth_tools": len(counts_per_tool)},
)
else:
sub_metrics["auth_num_calls_on_failure"] = MetricScore(
name=f"{parent_name}.auth_num_calls_on_failure",
score=round(avg_calls, 4),
normalized_score=None,
details={"calls_per_tool": counts_per_tool, "num_auth_tools": len(counts_per_tool)},
sub_metrics["num_auth_calls"] = MetricScore(
name=f"{parent_name}.num_auth_calls",
score=round(avg_calls, 4),
normalized_score=None,
details={"num_auth_calls_per_tool": counts_per_tool, "num_auth_tools": len(counts_per_tool)},
)
else:
sub_metrics["num_auth_calls_on_failure"] = MetricScore(
name=f"{parent_name}.num_auth_calls_on_failure",
score=round(avg_calls, 4),
normalized_score=None,
details={"num_auth_calls_per_tool": counts_per_tool, "num_auth_tools": len(counts_per_tool)},

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Will change both!

Comment on lines +123 to +124
score=round(avg_calls, 4),
normalized_score=None,

@JosephMarinier JosephMarinier Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an idea, maybe for the future:
A metric like authentication_success_rate = len(counts_per_tool) / sum(counts_per_tool.values()) (equivalent to 1 / avg_calls) would convey the exact same information as auth_num_calls but with two benefits:

  1. It's normalized between 0 and 1.
  2. Higher is better, like most of our metrics.

Would these advantages be worth it?

Unfortunately, it wouldn't have the same satisfying symmetry with auth_num_calls_on_failure as auth_num_calls currently has.

EDIT:
Do we average this submetric over the dataset?
If so, one caveat: authentication_success_rate would convey the same information as avg_calls for each record, but not once averaged over the dataset.
For example, if the avg_calls for two-sample dataset are 1 and 5, the average would be 3 (a single high number can overwhelm the average), while the average of $\displaystyle \frac{1}{1}$ and $\displaystyle \frac{1}{5}$ is $0.6$, not $\displaystyle \frac{1}{3}$. I'm not sure which is preferable; it depends on what we're trying to spot (outlier presence vs. tendency).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an interesting idea, and yes, we do average sub-metrics automatically. We could add it as a new sub-metric to get access to the 2 averages (with/without outliers). But I would not replace the existing one. Being able to compare with and without failure is exactly the goal I had in mind with these sub-metrics (for example, we want to know if the model tends to try more when it succeeds). For your point about higher is better, it's not true for sub-metrics; most of them have lower is better, because we often compute the rate of something bad, like how many conversations violate policies.

@JosephMarinier JosephMarinier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! This looks good to me. 👍

I only shared a few thoughts, maybe for future work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants