Telemetry of skills actually used - #306520
Closed
AbhitejJohn wants to merge 1 commit into
Closed
Conversation
Contributor
Author
|
Tagging @aeschli for help with reviewing this. It builds on the previous change that was merged last Friday. I've ensured this follows the code suggestions we had for the other PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an internal “tool completed successfully” event to the chat tools service and uses it to log telemetry when a model actually reads a skill’s SKILL.md via the readFile tool (including provenance and a content hash).
Changes:
- Added
onDidCompleteToolInvocationtoILanguageModelToolsServicewith a newIToolCompletedEventpayload. - Emitted the completion event from the browser implementation after successful tool invocation.
- Added
SkillContentReadTelemetryworkbench contribution that listens forreadFilecompletions targetingSKILL.mdand logsskillContentReadtelemetry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/test/common/tools/mockLanguageModelToolsService.ts | Updates the mock tools service to expose/fire the new completion event for tests. |
| src/vs/workbench/contrib/chat/common/tools/languageModelToolsService.ts | Extends the public interface with IToolCompletedEvent and onDidCompleteToolInvocation. |
| src/vs/workbench/contrib/chat/browser/tools/languageModelToolsService.ts | Emits onDidCompleteToolInvocation on successful tool execution. |
| src/vs/workbench/contrib/chat/browser/skillContentReadTelemetry.ts | New contribution that filters readFile calls to SKILL.md, resolves provenance, and logs telemetry including a content hash. |
| src/vs/workbench/contrib/chat/browser/chat.contribution.ts | Registers the new telemetry workbench contribution. |
AbhitejJohn
force-pushed
the
aajohn/skill-content-read
branch
from
March 30, 2026 23:31
d66a0cd to
3461b8b
Compare
- Add IToolCompletedEvent interface and onDidCompleteToolInvocation event to ILanguageModelToolsService - Fire event in LanguageModelToolsService after successful tool invocation - Add SkillContentReadTelemetry contribution that subscribes to the event - Filter to only readFile calls targeting SKILL.md files (cheap endsWith check) - Resolve skill match via cached path map, log provenance + content hash telemetry - Update mock service for testing
AbhitejJohn
force-pushed
the
aajohn/skill-content-read
branch
from
March 30, 2026 23:58
3461b8b to
231e0c5
Compare
Contributor
|
@AbhitejJohn I know this code base is not pretty and hard to explain, but the read tool is contributed from the extension. Core shouldn't 'know of the tool. I suggest to move the telemetry there. There's already some reporting. |
Contributor
Author
|
@aeschli : Thanks a bunch for pointing me to that codebase again. I should have picked up on it from your previous comment. |
github-merge-queue Bot
pushed a commit
to microsoft/vscode-copilot-chat
that referenced
this pull request
Apr 1, 2026
…4884) * Add skillContentRead telemetry event for skill provenance tracking Adds a new skillContentRead telemetry event fired when the read_file tool successfully reads a skill file. Separate from readFileToolInvoked and deferred via queueMicrotask to stay off the critical path. Captures: skillNameHash, skillStorage (extension/internal/user/local), extensionIdHash, extensionVersion, contentHash (GH event), plus plaintext skillName, skillPath, extensionId (enhanced GH event). Addresses microsoft/vscode#306520 (comment) * Address review: check skill before getText, use non-caching hash for content - Move getSkillInfo/getExtensionSkillInfo check before getText() so non-skill reads don't materialize document text unnecessarily - Export createSha256HashSyncInsecure and use it for contentHash to avoid retaining full file content in the global hash cache - Fix TS2345: new Promise<void>(resolve => queueMicrotask(() => resolve())) * Address PR review comments: consolidate telemetry, add internal MSFT event - Merge sendSkillContentReadTelemetry into sendReadFileTelemetry to reuse already-computed extensionSkillInfo/skillInfo (pwang347 feedback) - Remove queueMicrotask since sendReadFileTelemetry is already async fire-and-forget (pwang347 feedback) - Switch back to getCachedSha256Hash for content hash - skill files are small and rarely change so caching is appropriate (pwang347 feedback) - Add sendInternalMSFTTelemetryEvent with plaintext properties (digitarald) - Extract shared plaintextProps to reduce duplication across GH, enhanced GH, and internal MSFT events - Revert createSha256HashSyncInsecure export in crypto.ts (no longer needed) * Address PR feedback: merge skill telemetry into sendReadFileTelemetry, use hash() from vscode core, add internal MSFT event, remove queueMicrotask * Move skillStorage classification into platform layer Borrowed from #4914: adds SkillStorage enum and ISkillInfo interface to the platform layer, refactors _matchInstructionLocationsFromSkills to tag storage provenance (Personal/Workspace/Extension/Internal) at discovery time. Removes local getSkillStorage() from ReadFileTool. Also adds isSkillMdFile(uri) guard so skillContentRead only fires for SKILL.md files, not other assets in skill folders. Switches skillContentRead hashes from SHA-256 to vscode core's hash() for consistency with skillLoadedIntoContext event.
joaomoreno
pushed a commit
that referenced
this pull request
Apr 7, 2026
…4884) * Add skillContentRead telemetry event for skill provenance tracking Adds a new skillContentRead telemetry event fired when the read_file tool successfully reads a skill file. Separate from readFileToolInvoked and deferred via queueMicrotask to stay off the critical path. Captures: skillNameHash, skillStorage (extension/internal/user/local), extensionIdHash, extensionVersion, contentHash (GH event), plus plaintext skillName, skillPath, extensionId (enhanced GH event). Addresses #306520 (comment) * Address review: check skill before getText, use non-caching hash for content - Move getSkillInfo/getExtensionSkillInfo check before getText() so non-skill reads don't materialize document text unnecessarily - Export createSha256HashSyncInsecure and use it for contentHash to avoid retaining full file content in the global hash cache - Fix TS2345: new Promise<void>(resolve => queueMicrotask(() => resolve())) * Address PR review comments: consolidate telemetry, add internal MSFT event - Merge sendSkillContentReadTelemetry into sendReadFileTelemetry to reuse already-computed extensionSkillInfo/skillInfo (pwang347 feedback) - Remove queueMicrotask since sendReadFileTelemetry is already async fire-and-forget (pwang347 feedback) - Switch back to getCachedSha256Hash for content hash - skill files are small and rarely change so caching is appropriate (pwang347 feedback) - Add sendInternalMSFTTelemetryEvent with plaintext properties (digitarald) - Extract shared plaintextProps to reduce duplication across GH, enhanced GH, and internal MSFT events - Revert createSha256HashSyncInsecure export in crypto.ts (no longer needed) * Address PR feedback: merge skill telemetry into sendReadFileTelemetry, use hash() from vscode core, add internal MSFT event, remove queueMicrotask * Move skillStorage classification into platform layer Borrowed from #4914: adds SkillStorage enum and ISkillInfo interface to the platform layer, refactors _matchInstructionLocationsFromSkills to tag storage provenance (Personal/Workspace/Extension/Internal) at discovery time. Removes local getSkillStorage() from ReadFileTool. Also adds isSkillMdFile(uri) guard so skillContentRead only fires for SKILL.md files, not other assets in skill folders. Switches skillContentRead hashes from SHA-256 to vscode core's hash() for consistency with skillLoadedIntoContext event.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on #303110 — to give us insights on when a skill is actually used.
Broader level changes
New event: \onDidCompleteToolInvocation\ on \ILanguageModelToolsService\ — fires after successful tool invocation with the tool result. Added a subscriber \SkillContentReadTelemetry\ , filters to
eadFile\ calls targeting \SKILL.md\ files, resolves skill provenance, and logs telemetry
The telemetry logged is similar to what was captured in the other PR. Additionally we also capture a hash of skill content.
In terms of optimization, the cache of skills that we read is scoped to a session and is populated once on first SKILL.md read.