-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Add project telemetry #16050
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add project telemetry #16050
Conversation
src/compiler/commandLineParser.ts
Outdated
* Produces a cleaned version of compiler options with personally identifiying info (aka, paths) removed. | ||
* Also converts enum values back to strings. | ||
*/ | ||
export function convertCompilerOptionsForTelemetry(opts: ts.CompilerOptions): ts.CompilerOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@internal
?
src/server/editorServices.ts
Outdated
readonly dts: number; | ||
} | ||
|
||
function getFileStats(fileNames: string[]): FileStats { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just use countEachFileTypes
We also should add the new event in protocol.ts |
protocol.ts already has a "telemetry" event, should we use that? Or change the event name to "projectTelemetry"? |
i would add a new one. @mjbvz any preferences here? |
@Andy-MS we should also set |
src/server/editorServices.ts
Outdated
@@ -1029,9 +1037,19 @@ namespace ts.server { | |||
const data: ProjectInfoTelemetryEventData = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for completeness lets also include include
, exclude
and compileOnSave
@mhegazy It may be more consistent to stick with a single |
src/server/editorServices.ts
Outdated
export interface ProjectInfoTypeAcquisitionData { | ||
readonly enable: boolean; | ||
// Actual values of include/exclude entries are scrubbed. | ||
readonly include: Array<"">; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not this be like include
and exclude
and be boolean | undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In contrast to a tsconfig "include", typeAcquisition include can apparently be provided by an external config. It also appears to be getting automatically set to an empty array if not explicitly provided.
We could make it be a boolean
that is equal to whether the array is non-empty though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment about typingsOptions.include and typingsOptions.exclude.
A few more pieces of info that just came to mind:
|
src/server/editorServices.ts
Outdated
readonly files: boolean | undefined; | ||
readonly include: boolean | undefined; | ||
readonly exclude: boolean | undefined; | ||
readonly compileOnSave: boolean; | ||
readonly typeAcquisition: ProjectInfoTypeAcquisitionData; | ||
|
||
/** Is 'undefined' if config file name is not tsconfig or jsconfig. */ | ||
readonly configFileName: "tsconfig.json" | "jsconfig.json" | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined is not going to survive in the json serialization. consider adding "other"
value.
This adds a telemetry event the first time a project is opened in a session.
The event contains a count of files by extension and scrubbed compiler options.