Add gen_ai.skill.* attributes to the execute tool span - #498
filip-kopyt wants to merge 49 commits into
Conversation
Pull request dashboard statusWaiting on reviewers · refreshed 2026-09-18 12:38 UTC Review the latest changes. Status above doesn't look right?
|
There was a problem hiding this comment.
🟡 Changes recommended
Moderate correctness issues remain in ADK failure handling and Agent Framework skill resolution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds gen_ai.skill.* telemetry to execute_tool spans for Agent Skills operations.
Changes:
- Defines six skill attributes and span requirements.
- Adds Google ADK and Agent Framework reference scenarios.
- Updates generated documentation, coverage data, reports, and changelog.
File summaries
| File | Description |
|---|---|
reference/scenarios/google-adk/skills/code-review/SKILL.md |
Defines the ADK test skill. |
reference/scenarios/google-adk/skills/code-review/scripts/run_checks.py |
Adds the bundled test script. |
reference/scenarios/google-adk/skills/code-review/references/review_policy.md |
Adds the bundled test resource. |
reference/scenarios/google-adk/scenario.py |
Exercises and traces ADK skill tools. |
reference/scenarios/google-adk/README.md |
Documents ADK skill coverage. |
reference/scenarios/google-adk/data.json |
Captures emitted ADK telemetry. |
reference/scenarios/agent-framework/skills/code-review/SKILL.md |
Defines the Agent Framework test skill. |
reference/scenarios/agent-framework/skills/code-review/scripts/run_checks.py |
Adds the bundled test script. |
reference/scenarios/agent-framework/skills/code-review/references/review_policy.md |
Adds the bundled test resource. |
reference/scenarios/agent-framework/scenario.py |
Exercises and traces SkillsProvider. |
reference/scenarios/agent-framework/README.md |
Documents Agent Framework coverage. |
reference/scenarios/agent-framework/data.json |
Captures emitted Agent Framework telemetry. |
reference/reports/execute-tool-span.md |
Updates library coverage reporting. |
model/gen-ai/spans.yaml |
Attaches skill attributes to tool spans. |
model/gen-ai/registry.yaml |
Defines the new skill attributes. |
docs/registry/attributes/gen-ai.md |
Updates the generated attribute registry. |
docs/gen-ai/gen-ai-spans.md |
Updates generated span documentation. |
docs/gen-ai/gen-ai-agent-spans.md |
Documents Agent Skills tracing. |
changelog.d/+.enhancement.md |
Adds the enhancement release note. |
Review details
Suppressed comments (1)
docs/gen-ai/gen-ai-agent-spans.md:1078
- This row presents
gen_ai.skill.script.exit_codeas part of every script execution, but the span model makes it conditional on the tool reporting an exit code, and the Agent Framework reference explicitly cannot capture one. Mark the exit code as conditional so this guide does not overstate the requirement.
| Run a bundled script | the above, plus `gen_ai.skill.script.path` and `gen_ai.skill.script.exit_code` |
- Files reviewed: 19/19 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved security, semantic, and error-handling issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
model/gen-ai/registry.yaml:1049
- This gives conflicting values when a framework normalizes names. Agent Framework resolves names case-insensitively, so a call for
CODE-REVIEWresolves to frontmattercode-review: the first sentence says to record the former, while the second says it is the latter. The scenarios implement the requested value, so remove the canonical-name claim or redefine and update them to emit the resolved name.
model/gen-ai/spans.yaml:638 - These conditions overlap. ADK's
load_skill_resourceacceptsscripts/..., so reading a script file would require both path attributes even though the stage table assignsscript.pathonly to execution. Tie each condition to the action instead of the path's type.
docs/gen-ai/gen-ai-agent-spans.md:1078
- The table presents every listed attribute as present for each stage, but the normative requirements make description and source conditional on successful resolution and make exit code conditional on the tool reporting it. The new Agent Framework scenario demonstrates the latter by intentionally omitting exit code. Keep this overview consistent with those conditions.
| Load a skill's instructions | `gen_ai.skill.name`, `gen_ai.skill.description`, `gen_ai.skill.source.uri` |
| Read a bundled resource | the above, plus `gen_ai.skill.resource.path` |
| Run a bundled script | the above, plus `gen_ai.skill.script.path` and `gen_ai.skill.script.exit_code` |
reference/scenarios/google-adk/scenario.py:460
gen_ai.tool.call.resultis defined only for successful executions (model/gen-ai/registry.yaml:524), but this line runs beforeerror_codeis checked. Both failure phases therefore attach an error payload as a successful result. Derive the error first and set the result only in the non-error branch.
span.set_attribute("gen_ai.tool.call.result", json.dumps(result, default=str))
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate telemetry and sensitive-data guidance issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
docs/gen-ai/gen-ai-agent-spans.md:1078
- This table presents
gen_ai.skill.script.exit_codeas present on every script run, but the model makes it conditional and the Agent Framework reference explicitly cannot capture it (reference/scenarios/agent-framework/scenario.py:215-221). Mark the exit code as conditional so this guide does not contradict the convention and its own coverage.
| Run a bundled script | the above, plus `gen_ai.skill.script.path` and `gen_ai.skill.script.exit_code` |
docs/gen-ai/gen-ai-agent-spans.md:1067
- The first stage above is skill discovery, but not every framework implements discovery as a tool call. ADK has a separate
list_skillstool, while Agent Framework advertises metadata through its context provider. Saying that all three stages use the three listed tools incorrectly implies that discovery always produces anexecute_toolspan. Limit this claim to instruction loading, resource reads, and script execution.
Frameworks implement all three stages by exposing a small set of tools to the
model. The names differ — Google ADK offers `load_skill`, `load_skill_resource`
and `run_skill_script`; Microsoft Agent Framework offers `load_skill`,
`read_skill_resource` and `run_skill_script` — but the operations are the same.
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Non-zero ADK skill-script exits must be recorded as failed tool executions before approval.
Review details
Suppressed comments (6)
docs/gen-ai/gen-ai-agent-spans.md:1067
- The preceding paragraph defines the first stage as advertising skill names and descriptions, but Agent Framework performs that by injecting context instructions, not through a tool. Saying all three stages are tools also conflicts with the following table, which instead lists load, resource-read, and script-run operations. Describe those tool operations directly.
Frameworks implement all three stages by exposing a small set of tools to the
model. The names differ — Google ADK offers `load_skill`, `load_skill_resource`
and `run_skill_script`; Microsoft Agent Framework offers `load_skill`,
`read_skill_resource` and `run_skill_script` — but the operations are the same.
docs/gen-ai/gen-ai-agent-spans.md:1078
- This table presents every listed attribute as present for each stage, but the model makes
descriptionandsource.uriconditional on availability andscript.exit_codeconditional on the tool reporting it. The Agent Framework scenario intentionally cannot emit the exit code. Preserve those conditions here so this guidance does not conflict with the normative table.
| Load a skill's instructions | `gen_ai.skill.name`, `gen_ai.skill.description`, `gen_ai.skill.source.uri` |
| Read a bundled resource | the above, plus `gen_ai.skill.resource.path` |
| Run a bundled script | the above, plus `gen_ai.skill.script.path` and `gen_ai.skill.script.exit_code` |
reference/scenarios/agent-framework/README.md:15
skillsis not a semantic operation;SkillsProviderexecutes these asexecute_tool. Keeping a separate row makes this operation table contradict the convention and overstate the set of operations covered. Fold the detail intoexecute_tool.
| skills | Yes — `SkillsProvider` exposes the skill lifecycle as tools | ✅ Implemented |
reference/scenarios/google-adk/README.md:14
skillsis not a semantic operation; these calls remainexecute_toolspans. A separate row makes the operation coverage table imply the separate skill operation that this PR explicitly avoids. Fold this coverage into the existingexecute_toolrow.
| skills | Yes — `SkillToolset` runs the skill tools | ✅ Implemented |
reference/scenarios/google-adk/scenario.py:462
- ADK 2.8.0 returns
exit_codeandtimed_outfor an environment-run script without anerror_code; a non-zero exit therefore takes the success branch here and leaves the span status anderror.typeunset. Treat a non-zero exit as a skill-script execution error so the new exit-code attribute does not hide a failed tool execution.
error_type = result.get("error_code") if isinstance(result, dict) else None
reference/scenarios/google-adk/scenario.py:406
- The PR description says this change moves the scenario to google-adk 2.8.0 and validates the unchanged scenario against it, but neither this scenario's
pyproject.tomlnoruv.lockchanges in the PR; the current manifest already pins 2.8.0. Remove or correct that upgrade paragraph so the description matches the diff.
from google.adk.skills import load_skill_from_dir
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Balanced
lmolkova
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Left some high-level design comments.
e60f2be to
1c8b384
Compare
Frameworks expose Agent Skills to the model as tools — one to load a skill's instructions, one to read a bundled resource, one to run a bundled script — so instrumentation already observes skill use at the tool-execution boundary. Qualify that span with gen_ai.skill.* rather than defining a skill span, and key the convention on the attributes rather than on any framework's tool names. Reference scenarios cover the three stages in Google ADK (SkillToolset) and Microsoft Agent Framework (SkillsProvider), including a load that resolves no skill and a script that was never found, which is where the "recorded whether or not it resolves" wording is exercised. The ADK scenario moves to google-adk 2.8.0 for the skills toolset. That upgrade changes no telemetry on its own: the scenario emits the same data.json under the new version as under 2.6.2.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…deduplicate attributes, rewrite description of skill URI
Co-authored-by: Liudmila Molkova <neskazu@gmail.com>
Co-authored-by: Liudmila Molkova <neskazu@gmail.com>
Co-authored-by: Liudmila Molkova <neskazu@gmail.com>
Co-authored-by: Liudmila Molkova <neskazu@gmail.com>
Co-authored-by: Liudmila Molkova <neskazu@gmail.com>
Co-authored-by: Liudmila Molkova <neskazu@gmail.com>
Co-authored-by: Liudmila Molkova <neskazu@gmail.com>
c5eb9f9 to
3f6f2c1
Compare
|
CI failing due to github issues. |
This comment has been minimized.
This comment has been minimized.
|
/dashboard route:reviewers |
|
@filip-kopyt, this pull request was routed to reviewers. The handoff remains active across pushes until newer actionable human feedback arrives. Top-level feedback through this request will not return; unresolved review threads remain open. |
Description
An Agent Skill is a folder of instructions plus optional bundled resources and scripts that an agent loads on demand. This describes that use with attributes on the
execute_toolspan rather than with a span type of its own, so a load looks likeexecute_tool load_skillcarryinggen_ai.skill.name=code-reviewandgen_ai.skill.source.uri=file:///opt/skills/code-review.Motivation
Incident triage. Loading a skill injects a body of instructions into the agent's context, so when an agent starts doing the wrong thing the skill it loaded is often the cause. Right now that is invisible. The call shows up as a tool named
load_skillwith opaque arguments, and nothing records which skill resolved, where it came from, or whether its bundled script failed. Skill authors have the same blind spot from the other side, with no way to see whether the skills they publish get picked up or whether their scripts fail in the field.Keying the convention on attributes rather than on tool names matters because the names differ per framework. Google ADK offers
load_skill,load_skill_resourceandrun_skill_script. Microsoft Agent Framework offersload_skill,read_skill_resourceandrun_skill_script. A consumer that matches on tool names has to learn every framework's spelling, and gets nothing at all for a framework it has not seen.gen_ai.skill.source.uricarries identity. The format requires a skill name to be unique within one agent's skill set, but across a fleet the same name can be served from different sources, so the URI is what tells them apart.Prior art: the Agent Skills format at https://agentskills.io, implemented by ADK's
SkillToolsetand Agent Framework'sSkillsProvider. Both expose the same three stages as tools, which is why instrumentation can observe them at a boundary that already exists.Prototype
reference/scenarios/google-adkandreference/scenarios/agent-frameworkrun all three stages through each framework's own tool loop, so the spans come from the library's tool-execution entry point rather than from scenario code wrapped around it. Additionally ADK exercises the two failure cases the attribute wording depends on, a load naming a skill that does not exist and a script the skill does not bundle. Those are what make "recorded whether or not it resolves" testable rather than aspirational.gen_ai.skill.script.exit_codeis emitted by ADK only. Agent Framework hands file-based script execution to an application-supplied runner and takes back whatever that returns, so the process exit status is not part of the framework's contract and generic instrumentation there has nothing to read.The ADK scenario moves to google-adk 2.8.0 for the skills toolset. That upgrade emits identical telemetry on its own, verified by running the unchanged scenario against it, so every
data.jsonchange here comes from the new skill coverage.Checklist
changelog.d/for any change to the conventions that a consumer would care about. Editorial changes (typos, pure rewording, repo tooling) don't need an entry.