Rename skillContentRead telemetry properties to use 'skill' prefix#311945
Merged
pwang347 merged 1 commit intomicrosoft:mainfrom Apr 22, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates skillContentRead telemetry property names to consistently use a skill* prefix (per data team guidance), and updates tests accordingly. It also adds a workbench-side SkillContentReadTelemetry implementation + test intended to mirror upstream behavior.
Changes:
- Renamed
skillContentReadGH telemetry properties in the CopilotreadFiletool (extensionIdHash→skillExtensionIdHash, etc.). - Updated Copilot extension tests to assert the renamed telemetry properties.
- Added a workbench
SkillContentReadTelemetrycontribution and a corresponding unit test (currently not aligned with the repo’sILanguageModelToolsServiceAPI).
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/test/browser/skillContentReadTelemetry.test.ts | Adds tests for workbench skillContentRead telemetry using renamed skill* properties (but currently references non-existent tool service APIs/types). |
| src/vs/workbench/contrib/chat/browser/skillContentReadTelemetry.ts | Adds workbench telemetry emission for skill content reads using renamed skill* properties (but currently references non-existent tool service APIs/types). |
| extensions/copilot/src/extension/tools/node/test/readFile.spec.tsx | Updates assertions and test title strings to the renamed telemetry properties. |
| extensions/copilot/src/extension/tools/node/readFileTool.tsx | Renames skillContentRead telemetry payload properties to the new skill*-prefixed names. |
Copilot's findings
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/chat/browser/skillContentReadTelemetry.ts:35
ILanguageModelToolsServicecurrently exposesonDidInvokeToolbut does not haveonDidCompleteToolInvocation, so this subscription won’t compile. Either switch to the existing event(s) and derive completion via the tool invocation/state model, or extend the service interface + implementation to add a completion event.
this._register(toolsService.onDidCompleteToolInvocation(e => this._onToolCompleted(e)));
this._register(this._promptsService.onDidChangeSkills(() => {
src/vs/workbench/contrib/chat/test/browser/skillContentReadTelemetry.test.ts:95
- This test calls
mockToolsService.fireOnDidCompleteToolInvocation(...), butMockLanguageModelToolsServiceinsrc/vs/workbench/contrib/chat/test/common/tools/mockLanguageModelToolsService.tsonly providesfireOnDidInvokeTooland there is no completion event inILanguageModelToolsService. Update the test to use the actual service API (or add the completion event + mock support as part of this PR).
async function fireAndAwait(event: IToolCompletedEvent): Promise<void> {
mockToolsService.fireOnDidCompleteToolInvocation(event);
// Allow the fire-and-forget async telemetry to complete
await new Promise<void>(resolve => setTimeout(resolve, 50));
- Files reviewed: 4/4 changed files
- Comments generated: 2
60c3b5b to
1144448
Compare
pwang347
approved these changes
Apr 22, 2026
roblourens
approved these changes
Apr 22, 2026
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Renames telemetry properties in the
skillContentReadevent to add askillprefix, aligning property names with the event name to reduce ambiguity in the collected data. This was suggested by folks on the data team.Property Renames
GH telemetry event (
sendGHTelemetryEvent)Enhanced GH + Internal MSFT events (plaintext props)
Properties that already had the
skillprefix (skillName,skillPath,skillStorage,skillNameHash) are unchanged.Testing