Add telemetry for open JS files#23833
Conversation
sheetalkamat
left a comment
There was a problem hiding this comment.
I still think sending openFileStats every file open is better than storing info on disk and sending in next session.
| this.addFileOrFolderInFolder(baseFolder, folder); | ||
| } | ||
|
|
||
| createDirectoryRecursively(directoryName: string) { |
There was a problem hiding this comment.
Dont need this. you can use ensureFileOrFolder
|
I think we shouldn't send the telemetry for a project that has no |
| if (project && project.getCompilationSettings().checkJs) return; | ||
|
|
||
| const stats: Mutable<OpenFileStats> = { js: 0, checkJs: 0 }; | ||
| this.filenameToScriptInfo.forEach(scriptInfo => { |
There was a problem hiding this comment.
We arent sending stats from start of the session to this point in time here. I thought that was the indent of this event. Do we need this info only for open files or all files ?
|
|
||
| this.printProjects(); | ||
|
|
||
| this.telemetryOnOpenFile(project, fileName, info); |
There was a problem hiding this comment.
project here is could not be project for file being opened. Eg. configFile search results in creating configured project but that project doesnt include info. You would always want to do info.getDefaultProject() to correctly get the default project for the info.
| } | ||
|
|
||
| private telemetryOnOpenFile(project: ConfiguredProject | ExternalProject | undefined, fileName: NormalizedPath, info: ScriptInfo): void { | ||
| if ( |
There was a problem hiding this comment.
You would just want to assert that: info.containingProjects.length !== 0
There was a problem hiding this comment.
Hm, that assertion fails at the test works when project root is used with case-sensitive system. It works with the case-insensitive version of that test -- is this a bug?
There was a problem hiding this comment.
because you arent using info.path as key?
| // Won't be able to get sourceFile without containingProjects | ||
| info.containingProjects.length === 0 || | ||
| // If we've already seen the file, no need to send a new evnt. | ||
| !addToSeen(this.allJsFilesForOpenFilesTelemetry, fileName)) { |
There was a problem hiding this comment.
why not instead of just storing "seen" as true store value true or false as checkJsDirective. Then at all times the count of values === true and we dont need to store tsCheckCountForOpenFilesTelemetry separately.
| return; | ||
| } | ||
|
|
||
| if (info.getDefaultProject().getSourceFile(info.path).checkJsDirective) { |
There was a problem hiding this comment.
Do we care if checkJsDirective changes? Project settings change to enable checkJS? and hence enabling checkJSDirective?
There was a problem hiding this comment.
I think we'll still get good data in the aggregate, assuming these change rarely. We'll get the updated data the next time you use the editor.
| // We are only sending stats for JS files | ||
| !info.isJavaScript() || | ||
| // If we've already seen the file, no need to send a new event. | ||
| this.allJsFilesForOpenFilesTelemetry.has(fileName)) { |
There was a problem hiding this comment.
Should use filePath as a key instead of fileName since the caseInsensitive system: "filename" === "fileName"
| if ( | ||
| !this.eventHandler || | ||
| // Don't send for a project with 'checkJs' enabled globally. | ||
| info.getDefaultProject().getCompilationSettings().checkJs || |
There was a problem hiding this comment.
Shouldnt check this here since you want to add to allJsFilesForOpenFilesTelemetry if the info is js but project doesnt have checkJS
| } | ||
|
|
||
| export type ProjectServiceEvent = ProjectsUpdatedInBackgroundEvent | ConfigFileDiagEvent | ProjectLanguageServiceStateEvent | ProjectInfoTelemetryEvent; | ||
| export interface OpenFileStats { |
There was a problem hiding this comment.
i thought we will just send an event on every new file .js we see in openfile, and will have a flag whether it has checkjs or not. we can do the counting at the server side. no need for counting here.
| } | ||
|
|
||
| const info: OpenFileInfo = { checkJs: !!scriptInfo.getDefaultProject().getSourceFile(scriptInfo.path).checkJsDirective }; | ||
| this.eventHandler({ eventName: OpenFileInfoTelemetryEvent, data: { info } }); |
There was a problem hiding this comment.
dont you need to send the open file name so that at vscode side they can create a map of seen files and get correct count instead of count = 3 when opening same file 3 times?
There was a problem hiding this comment.
If we open the same file 3 times we should still only send one event, thanks to this.allJsFilesForOpenFileTelemetry.
Fixes #23178
@sheetalkamat mentioned that instead of saving to a file, we could send multiple events and have the editor use only the last event, which would simplify this PR a lot. Pushing this as-is for now but we should consider changing it.