Add telemetry for Language Server download, extraction, and analysis (startup) time. #2593

d3r3kk · 2018-09-14T19:29:13Z

Completes #2461

Note: I had to add telemetry to the ProgressReporting class to figure out when the LS reports that it is completed the analysis for the workspace. If there is a better way for me to do this, let me know! I believe that once the analysis reporting that the LS is doing today is fixed up, the two telemetry measurements I am sending (PYTHON_LANGUAGE_SERVER_ANALYSISTIME and PYTHON_LANGUAGE_SERVER_STARTUP) should be pretty much equal.

- Simple fix to add telemetry in the existing manner - Testing will have to be done a different way

- versus hard-coding the string in multiple places.

- Move telemetry into the languageServer class and out of downloader. - Add telemetry properties. - Add telemetry for languageServer startup time.

- last change introduced a bug to do with async/await order - moved sendTelemetryWhenDone up a level to compensate

- Telemetry will show success/fail on the analysis time this way. - Alphabetize PYTHON_LANGUAGE_SERVER_... constants

DonJayamanne · 2018-09-14T19:55:28Z

src/client/activation/downloader.ts

-            throw new Error(err);
+            try {
+                await this.downloadFile(downloadUri, 'Downloading Microsoft Python Language Server... ')
+                    .then((tmpFilePath: string) => {


You don't need the .then. Just use the await, and next line initialize stats.
I.e. you can revert the change.

DonJayamanne · 2018-09-14T19:56:08Z

src/client/activation/downloader.ts

+            timer.reset();
+            try {
+                await this.unpackArchive(context.extensionPath, localTempFilePath)
+                    .then(() => {


no need of .then.
Just initialize the stats immediately after await line

DonJayamanne · 2018-09-14T19:58:58Z

src/client/telemetry/types.ts

@@ -57,6 +57,20 @@ export type DebuggerPerformanceTelemetry = {
    duration: number;
    action: 'stepIn' | 'stepOut' | 'continue' | 'next' | 'launch';
 };
+export type LanguageServerInstallTelemetry = {
+    downloadSuccess: boolean;


all of these properties should be nullable.
Default values cannot be false. Cuz its misleading to have downloadSuccess = false and extractSuccess = false. Cuz if download fails, then extraction was never attempted. Hence it should not be false, else we end up with skewed data.

DonJayamanne · 2018-09-14T19:59:29Z

src/client/activation/downloader.ts

-    public async downloadLanguageServer(context: IExtensionContext): Promise<void> {
+    public async downloadLanguageServer(context: IExtensionContext): Promise<LanguageServerInstallTelemetry> {
+        const stats: LanguageServerInstallTelemetry = {
+            downloadMs: -1,


Please remove the time = -1.
Default should be undefined

DonJayamanne · 2018-09-14T20:05:51Z

src/client/activation/downloader.ts

@@ -41,22 +43,48 @@ export class LanguageServerDownloader {
        return DownloadLinks[platformString];
    }

-    public async downloadLanguageServer(context: IExtensionContext): Promise<void> {
+    public async downloadLanguageServer(context: IExtensionContext): Promise<LanguageServerInstallTelemetry> {


You should be able to revert the changes to this method and use captureTelemetry decorator in telemetry/index.ts

All you'll need to do is add the decorator the to downloadFile and unpackArchieve methods.
You might want to tweak the captureTelemetry function to capture failures (minor tweak).

With that you'll be able to reduce the changes to the code, with the power of decorators :)

DonJayamanne · 2018-09-14T20:13:56Z

src/client/activation/languageServer.ts

+            };
+            sendTelemetryWhenDone(
+                PYTHON_LANGUAGE_SERVER_STARTUP,
+                this.startLanguageClient()


Again, you should be able to use the decorator here.
Please note, you cannot just send the entire error object via telemetry.
We need to be aware of GDPR, sending error object could contain stack traces, that could contain PII (user name, etc).

DonJayamanne · 2018-09-14T20:15:08Z

src/client/activation/progress.ts


 export class ProgressReporting {
  private statusBarMessage: Disposable | undefined;
  private progress: Progress<{ message?: string; increment?: number }> | undefined;
  private progressDeferred: Deferred<void> | undefined;
+  private progressTimer: StopWatch | undefined;


Or a more simpler way is private progressTimer?: StopWatch;
No changes necessary here, just letting you know there's a shorter syntax for nullable.

DonJayamanne · 2018-09-14T20:16:08Z

src/client/activation/progress.ts

+      this.progressTimer = new StopWatch();
+      this.progressTimeout = setTimeout(
+        // tslint:disable-next-line:no-any
+        (...args: any[]) => {


please change to argumentless anonymous func as follows:

this.progressTimeout = setTimeout(()=> { ... });

I.e. you do not need (...args: any[]) = >

Can we move this callback into a separate function, e.g.
this.progressTimeOut = setTimeout(()=> this.handleProgressTimeout());

DonJayamanne · 2018-09-14T20:20:00Z

src/client/common/constants.ts

@@ -6,6 +6,8 @@ export const PYTHON = [
    { scheme: 'untitled', language: PYTHON_LANGUAGE }
 ];

+export const PVSC_EXTENSION_ID = 'ms-python.python';


Fixes microsoft#2461 (alternate fix to PR microsoft#2593) - Capture telemetry surrounds methods, minimizing change - Telemetry type can be altered with less code later. - Add success/fail props modifyer func to `captureTelemetry`

d3r3kk · 2018-09-15T05:29:49Z

Closing in favour of #2597.

Fixes microsoft#2461 (alternate fix to PR microsoft#2593) - Capture telemetry surrounds methods, minimizing change - Telemetry type can be altered with less code later. - Add success/fail props modifyer func to `captureTelemetry`

Fixes #2461 (alternate fix to PR #2593)

d3r3kk added 7 commits September 13, 2018 11:27

fix gitignore for different version venv folder

4f9ec1c

Send telemetry events for LS download & extract

2d37a88

- Simple fix to add telemetry in the existing manner - Testing will have to be done a different way

Add constant for the extension ID

438c247

- versus hard-coding the string in multiple places.

Further telemetry updates

9fa9c84

- Move telemetry into the languageServer class and out of downloader. - Add telemetry properties. - Add telemetry for languageServer startup time.

Correct issue with telemetry sending

8f50a66

- last change introduced a bug to do with async/await order - moved sendTelemetryWhenDone up a level to compensate

Add news

7b931c2

Add telemetry for the *reported* analysis time, and provide a timeout.

db736d2

- Telemetry will show success/fail on the analysis time this way. - Alphabetize PYTHON_LANGUAGE_SERVER_... constants

d3r3kk requested a review from DonJayamanne September 14, 2018 19:29

DonJayamanne requested changes Sep 14, 2018

View reviewed changes

d3r3kk mentioned this pull request Sep 15, 2018

Add telemetry for download, extract, and analyze. #2597

Merged

8 tasks

d3r3kk closed this Sep 15, 2018

d3r3kk added a commit that referenced this pull request Sep 18, 2018

Add telemetry for download, extract, and analyze. (#2597)

581c18d

Fixes #2461 (alternate fix to PR #2593)

d3r3kk deleted the telemetry_ext branch December 19, 2018 18:04

lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add telemetry for Language Server download, extraction, and analysis (startup) time. #2593

Add telemetry for Language Server download, extraction, and analysis (startup) time. #2593

Uh oh!

d3r3kk commented Sep 14, 2018 •

edited

Loading

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

DonJayamanne Sep 14, 2018

Uh oh!

d3r3kk commented Sep 15, 2018

Uh oh!

Uh oh!

Add telemetry for Language Server download, extraction, and analysis (startup) time. #2593

Add telemetry for Language Server download, extraction, and analysis (startup) time. #2593

Uh oh!

Conversation

d3r3kk commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d3r3kk commented Sep 15, 2018

Uh oh!

Uh oh!

d3r3kk commented Sep 14, 2018 •

edited

Loading