Fix incorrect telemetry for inline completions #291315
Conversation
… shown to the user
There was a problem hiding this comment.
Pull request overview
Adjusts inline completion lifetime telemetry so that “shown” and “rejected” are only reported when the suggestion was actually presented to the user.
Changes:
- Prevents
reportInlineEditShownfrom running once end-of-life has already been reported. - Downgrades end-of-life
RejectedtoIgnoredwhen the suggestion was never shown to the user.
| // A suggestion can only be "rejected" if it was actually shown to the user. | ||
| // If the suggestion was never shown, downgrade to "ignored". | ||
| if (reason.kind === InlineCompletionEndOfLifeReasonKind.Rejected && !this._didShow) { | ||
| reason = { kind: InlineCompletionEndOfLifeReasonKind.Ignored, userTypingDisagreed: false, supersededBy: undefined }; | ||
| } |
There was a problem hiding this comment.
The { kind: Ignored, userTypingDisagreed: false, supersededBy: undefined } literal is now duplicated (here and in the default reason assignment above). To avoid the two getting out of sync over time, consider factoring this into a small helper/const (e.g. const defaultIgnoredReason = ...) and reusing it in both places.
| // A suggestion can only be "rejected" if it was actually shown to the user. | ||
| // If the suggestion was never shown, downgrade to "ignored". | ||
| if (reason.kind === InlineCompletionEndOfLifeReasonKind.Rejected && !this._didShow) { | ||
| reason = { kind: InlineCompletionEndOfLifeReasonKind.Ignored, userTypingDisagreed: false, supersededBy: undefined }; | ||
| } |
There was a problem hiding this comment.
This change alters end-of-life classification (downgrading Rejected to Ignored when the suggestion was never shown). There are existing inline completions browser tests; please add a focused test that exercises stop('explicitCancel') (or equivalent) before the first reportInlineEditShown and asserts the provider receives Ignored (and that handleRejection is not called) to prevent regressions in telemetry/provider callbacks.
| public async reportInlineEditShown(commandService: ICommandService, updatedInsertText: string, viewKind: InlineCompletionViewKind, viewData: InlineCompletionViewData, editKind: InlineSuggestionEditKind | undefined, timeWhenShown: number): Promise<void> { | ||
| this.updateShownDuration(viewKind); | ||
|
|
||
| if (this._didShow) { | ||
| if (this._didShow || this._didReportEndOfLife) { | ||
| return; |
There was a problem hiding this comment.
updateShownDuration(viewKind) runs even when _didReportEndOfLife is already true. This can re-open _showStartTime / _showUncollapsedStartTime after end-of-life (e.g. setTimeout0-scheduled handleInlineSuggestionShown firing late), leaving timers running and potentially skewing internal duration state. Consider returning early when _didReportEndOfLife is true before calling updateShownDuration, while keeping the existing updateShownDuration call for the _didShow fast-path (e.g. if (_didReportEndOfLife) return; updateShownDuration(...); if (_didShow) return;).
See below for a potential fix:
if (this._didReportEndOfLife) {
return;
}
this.updateShownDuration(viewKind);
if (this._didShow) {
2 participants
Copilot Generated Description:Correct the telemetry reporting for inline completions that are not shown to the user, ensuring that only relevant events are tracked.