Conversation
…ernal users - Remove repoId from sendMSFTTelemetryEvent (sensitive data) - Gate diff computation behind isInternal check so external users only get lightweight metadata (repoType + headCommitHash) - Extract shared _resolveRepoContext() to deduplicate repo context resolution between _getRepoMetadata() and _getRepoInfoTelemetry() - Update tests to pass copilotTokenStore and fix assertions
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts repository telemetry emission to avoid sending sensitive repoId to external Microsoft telemetry and to reduce overhead for non-internal users by skipping diff computation and related work.
Changes:
- Gate full repo diff collection + internal telemetry behind
ICopilotTokenStore.copilotToken.isInternal. - For non-internal users, send a lightweight external telemetry payload containing only repo type + upstream commit hash (no
repoId, no diffs). - Update unit tests to pass the new
copilotTokenStoredependency and to assertrepoIdis absent from external telemetry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/extension/prompt/node/repoInfoTelemetry.ts | Adds internal-user gating via token store and introduces _resolveRepoContext() + _getRepoMetadata() to avoid diff work for external users. |
| src/extension/prompt/node/test/repoInfoTelemetry.spec.ts | Updates constructor arguments and adjusts assertions for external telemetry payload + internal telemetry gating. |
Comments suppressed due to low confidence (1)
src/extension/prompt/node/repoInfoTelemetry.ts:170
- For non-internal users,
_sendRepoInfoTelemetryalways returnsundefined, sosendBeginTelemetryIfNeedednever sets_beginTelemetryResult. As a result,sendEndTelemetry()will always skip sending thelocation: 'end'event for external users, even when begin telemetry was successfully sent. Consider returning a minimalRepoInfoTelemetryData(or separately tracking a success flag/result) for the external path so the end event can be emitted consistently when appropriate.
private async _sendRepoInfoTelemetry(location: 'begin' | 'end'): Promise<RepoInfoTelemetryData | undefined> {
const isInternal = !!this._copilotTokenStore.copilotToken?.isInternal;
if (isInternal) {
const repoInfo = await this._getRepoInfoTelemetry();
if (!repoInfo) {
return undefined;
}
const internalProperties: RepoInfoInternalTelemetryProperties = {
...repoInfo.properties,
location,
telemetryMessageId: this._telemetryMessageId
};
this._telemetryService.sendInternalMSFTTelemetryEvent('request.repoInfo', internalProperties, repoInfo.measurements);
return repoInfo;
}
const metadata = await this._getRepoMetadata();
if (metadata) {
this._telemetryService.sendMSFTTelemetryEvent('request.repoInfo', {
repoType: metadata.repoType,
headCommitHash: metadata.headCommitHash,
location,
telemetryMessageId: this._telemetryMessageId,
});
}
return undefined;
karthiknadig
previously approved these changes
Feb 26, 2026
| this._telemetryService.sendInternalMSFTTelemetryEvent('request.repoInfo', internalProperties, repoInfo.measurements); | ||
| const metadata = await this._getRepoMetadata(); | ||
| if (metadata) { | ||
| this._telemetryService.sendMSFTTelemetryEvent('request.repoInfo', { |
Member
There was a problem hiding this comment.
Quick question (that I don't know the answer to off hand) would you need a new name for this event? They might go into totally different tables, which would be ok. Just don't want the folks watching the existing request.repoInfo to get confused by a new event with less properties / data.
| if (metadata) { | ||
| this._telemetryService.sendMSFTTelemetryEvent('request.repoInfo', { | ||
| repoType: metadata.repoType, | ||
| headCommitHash: metadata.headCommitHash, |
Member
There was a problem hiding this comment.
Is this still useful information without the id?
TylerLeonhardt
approved these changes
Feb 26, 2026
IanMatthewHuff
approved these changes
Feb 26, 2026
Member
IanMatthewHuff
left a comment
There was a problem hiding this comment.
Added a comment or two, but the code change should resolve the perf issue as I saw it.
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.
Follow-up to #3956:
repoIdfrom external MSFT telemetry (sensitive data)isInternalcheck — external users only get lightweight metadata (repoType+headCommitHash)_resolveRepoContext()to deduplicate repo context resolutioncopilotTokenStoreparameter