Instrument accepted suggestion contributions #136922
Conversation
|
Put in the |
src/vs/editor/common/modes.ts
Outdated
| /** | ||
| * @internal | ||
| */ | ||
| _source?: string; |
There was a problem hiding this comment.
This basically duplicates _debugDisplayName but looses precision. Extensions like TypeScript register a couple of completion item providers (I believe 3 for JS and 3 for TS) and the debug display name allows you to tell them apart (with some chance of error). Like, you won't be able to differentiate the accept rate of JSDoc completions vs semantic completions etc.
There was a problem hiding this comment.
Hm, so I had originally kept them separate because the snippet provider's source could be more specific than the current display name. But given that's not actually happening yet I agree this is not needed.
|
|
||
| const resourceLoggedEvents = this._loggedEvents.get(model.uri) ?? new Set<string>(); | ||
| if (!this._loggedEvents.has(model.uri)) { | ||
| this._loggedEvents.set(model.uri, resourceLoggedEvents); |
There was a problem hiding this comment.
This is never being cleared so a potential, slow memory leak. It's also not clear why we gate the telemetry? You won't be able to say anything about the frequency of provider-usage because a single usage is enough, right?
There was a problem hiding this comment.
fyi - to reduce telemetry load (in case that's the intention here) I have used this pattern in the past:
. Basically only report telemetry every so many timesThere was a problem hiding this comment.
Trying to reduce cycles that don't immediately benefit the user in the core edit loop as much as possible, the modulo-gating strategy makes good sense.
|
|
||
| const providerId = event.item.provider._source ?? event.item.provider._debugDisplayName ?? 'unknownProvider'; | ||
| if (!resourceLoggedEvents.has(providerId)) { | ||
| resourceLoggedEvents.add(providerId); |
There was a problem hiding this comment.
Suggestion to extract this into a separate function, similar to this. Maybe also wrapped in a setTimeout or idleCallback to ensure this doesn't impact editor rendering perf (which seems unlikely)
Adding some telemetry to get a better understanding of what extensions produce helpful suggestions in a given context (language /file extension/ (hashed) file name).