Fix the gather survey #13086

DavidKutu · 2020-07-21T23:49:23Z

added 'gather stats' telemetry
mention the gather comments to update the python ext

added 'gather stats' telemetry mention the gather comments to update the python ext

codecov-commenter · 2020-07-21T23:54:57Z

Codecov Report

Merging #13086 into master will decrease coverage by 0.07%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master   #13086      +/-   ##
==========================================
- Coverage   59.84%   59.76%   -0.08%     
==========================================
  Files         671      671              
  Lines       36653    36686      +33     
  Branches     5153     5158       +5     
==========================================
- Hits        21936    21927       -9     
- Misses      13625    13663      +38     
- Partials     1092     1096       +4

Impacted Files	Coverage Δ
src/client/telemetry/index.ts	`82.35% <ø> (ø)`
src/client/datascience/commands/commandRegistry.ts	`34.09% <20.00%> (-0.02%)`	⬇️
src/client/common/utils/localize.ts	`96.13% <100.00%> (+0.02%)`	⬆️
src/client/datascience/constants.ts	`99.77% <100.00%> (+<0.01%)`	⬆️
...atascience/jupyter/jupyterSessionManagerFactory.ts	`29.62% <0.00%> (-12.48%)`	⬇️
src/client/common/utils/platform.ts	`64.70% <0.00%> (-11.77%)`	⬇️
src/client/datascience/crossProcessLock.ts	`79.41% <0.00%> (-11.77%)`	⬇️
...ient/datascience/jupyter/kernels/kernelSelector.ts	`70.45% <0.00%> (-9.42%)`	⬇️
...ient/datascience/jupyter/kernels/kernelSwitcher.ts	`75.55% <0.00%> (-9.06%)`	⬇️
src/client/activation/languageServer/activator.ts	`93.93% <0.00%> (-6.07%)`	⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf4f377...e8a0a9a. Read the comment docs.

greazer · 2020-07-22T00:19:22Z

package.nls.json

@@ -474,8 +474,8 @@
    "DataScience.findJupyterCommandProgressCheckInterpreter": "Checking {0}.",
    "DataScience.findJupyterCommandProgressSearchCurrentPath": "Searching current path.",
    "DataScience.gatherError": "Gather internal error",
-    "DataScience.gatheredScriptDescription": "# This file was generated by the Gather Extension.\n#\n#     The intent is that it contains only the code required to produce\n#     the same results as the cell originally selected for gathering.\n#     Please note that the Python analysis is quite conservative, so if\n#     it is unsure whether a line of code is necessary for execution, it\n#     will err on the side of including it.\n#\n# Please let us know if you are satisfied with what was gathered here:\n# https://aka.ms/gathersurvey\n\n",
-    "DataScience.gatheredNotebookDescriptionInMarkdown": "## Gathered Notebook\nGathered from ```{0}```\n\n|   |   |\n|---|---|\n|&nbsp;&nbsp;&nbsp|This notebook was generated by the Gather Extension. The intent is that it contains only the code and cells required to produce the same results as the cell originally selected for gathering. Please note that the Python analysis is quite conservative, so if it is unsure whether a line of code is necessary for execution, it will err on the side of including it.|\n\n**Are you satisfied with the code that was gathered?**\n\n[Yes](https://command:python.datascience.gatherquality?yes) [No](https://command:python.datascience.gatherquality?no)",
+    "DataScience.gatheredScriptDescription": "# This file was generated by the Gather Extension.\n# It will not work without the lastest version of the Python Extension.\n#\n#     The intent is that it contains only the code required to produce\n#     the same results as the cell originally selected for gathering.\n#     Please note that the Python analysis is quite conservative, so if\n#     it is unsure whether a line of code is necessary for execution, it\n#     will err on the side of including it.\n#\n# Please let us know if you are satisfied with what was gathered here:\n# https://aka.ms/gathersurvey\n\n",


Try to keep the sentences positive. So instead of "It will not work..." say "It requires the latest version of the python extension."

I think it would be legitimate to actually state the version number as well, as in "It requires at least version 2020.7.94776 of the Python extension."

Finally, isn't it possible to set a version on the dependency set? I.e. when you install gather can't it be made to fail or force an update to the Python extension if it's not at least a certain version? That would be the best.

its not possible to install a specific version

greazer · 2020-07-22T00:23:56Z

src/client/datascience/gather/gatherListener.ts

+
+            sendTelemetryEvent(Telemetry.GatherStats, undefined, {
+                linesAvailable: linesAvailable ? linesAvailable - cell.data.source.length : -1,
+                linesInGatheredCell: cell.data.source.length,


This is the number of lines in the cell to be gathered? Do we need this? I think knowing:

The total number of source lines being submitted to be analyzed for gather

The total number of cells being submitted to be analyzed for gather

The total number of lines generated for the gathered result

The total number of cells generated for the gathered result

is what I think we're looking for.

greazer · 2020-07-22T00:26:36Z

src/client/datascience/gather/gatherListener.ts

+            sendTelemetryEvent(Telemetry.GatherStats, undefined, {
+                linesAvailable: linesAvailable ? linesAvailable - cell.data.source.length : -1,
+                linesInGatheredCell: cell.data.source.length,
+                gatheredLines: slicedProgram.splitLines().length - cell.data.source.length


Why are we subtracting the cell data source length? If I recall correctly the cell data source is the source for the entire cell being gathered. After gathering this cell may not contain all the same lines it originally had. So subtracting the number of lines that were in the source from the number of lines gathered total will not always be accurate.

greazer · 2020-07-22T00:29:49Z

src/client/datascience/gather/gatherLogger.ts

+                let gatherCount: number | undefined = this.context.globalState.get('gatherCount');
+
+                if (gatherCount) {
+                    gatherCount += vscCell.data.source.length;


Is vscCell.data.source.length equal to the total number of lines in the program being submitted to be gathered, and not just for the cell? I don't remember.

just of the cell

greazer · 2020-07-22T00:30:45Z

src/client/telemetry/index.ts

@@ -2041,6 +2041,11 @@ export interface IEventNamePropertyMapping {
         */
        result: 'err' | 'script' | 'notebook' | 'unavailable';
    };
+    [Telemetry.GatherStats]: {
+        linesAvailable: number; // Code lines before executing gather, does not include the gathered cell.


I don't think this is what we want. See above.

rchiodo · 2020-07-22T18:48:04Z

src/client/datascience/gather/gatherListener.ts

@@ -255,4 +276,17 @@ export class GatherListener implements IInteractiveWindowListener {
            editBuilder.insert(new Position(editor.document.lineCount, 0), '\n');
        });
    }
+
+    private getNumberOfCells(program: string): number {


There's a function that can do this already. generateCellsFromString. You'd call that and just return the array lenth.

rchiodo · 2020-07-22T18:49:27Z

src/client/datascience/gather/gatherLogger.ts

+                    let gatherLinesCount: number | undefined = this.context.globalState.get('gatherLinesCount');
+                    let gatherCellsCount: number | undefined = this.context.globalState.get('gatherCellsCount');
+
+                    if (gatherLinesCount) {


Don't these two values start out as zero/undefined? Seems to me it will always make the global state zero for both.

That depends, does the gatherLogger constructor keep getting executed or does it happen once?

It happens once, but if the value is zero, you update the value to zero again.

Nevermind, I see the issue.

rchiodo · 2020-07-22T18:50:35Z

src/client/datascience/interactive-common/linkProvider.ts

@@ -52,8 +53,7 @@ export class LinkProvider implements IInteractiveWindowListener {
                        this.openFile(href);
                    } else if (href.startsWith('https://command:')) {
                        const temp: string = href.split(':')[2];
-                        const params: string[] =
-                            temp.includes('/?') && temp.includes(',') ? temp.split('/?')[1].split(',') : [];
+                        const params: string[] = temp.includes('/?') ? temp.split('/?')[1].split(',') : [];


Does this change break the reason the ',' was looked for before? Did you look to see what originally put this && condition here and make sure that still worked?

I didn't look for the specific case, but I tested having both a single and multiple parameters and that works as expected.

Yeah I just checked it, out of the 3 commands that use this, only the gather survey one uses parameters

rchiodo

⏲️

sonarqubecloud · 2020-07-22T20:32:47Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

* fix the gather survey added 'gather stats' telemetry mention the gather comments to update the python ext * oops * fix tests and address comments * update gather stats when resetting the kernel * set globalstate vars to 0 when we open vs code * fix gather stats telemetry

* Fix the gather survey (#13086) * fix the gather survey added 'gather stats' telemetry mention the gather comments to update the python ext * oops * fix tests and address comments * update gather stats when resetting the kernel * set globalstate vars to 0 when we open vs code * fix gather stats telemetry * fix tests * fix tests for real

* Update model.isTrusted on trust change (#12820) (#12823) * Reduce visual complexity of trust prompt (#12839) (#12847) * Port python 2.7 fix to release (#12877) * port color fix on collapse all (#12895) (#12897) * fix a color on collapse all (#12895) * update changelog * Merge fixes into July release (#12889) Co-authored-by: Timothy Ruscica <[email protected]> * Merge more fixes into july release (#12918) Co-authored-by: Joyce Er <[email protected]> * Port trust fixes (#12929) * Fix regressions in trusted notebooks (#12902) * Handle trustAllNotebooks selection * Fix bug where after trusting, UI didn't update * Recover from ENOENT due to missing parent directory when trusting notebook (#12913) * Disable keydown on native cells in untrusted notebooks (#12914) * Hide editor icons when editor is not a notebook (#12934) (#12935) * Check for hideFromUser before activating current terminal (#12942) (#12956) * Check for hideFromUser before activating current terminal * Add tests * Tweak logic * Port final trust fixes for release (#12965) * Only allow Enter / NumpadEnter w/o ctrl/shift/alt (#12939) * Send telemetry for notebook trust prompt selections (#12964) * Fixes for persisting trust (#12950) * Display survey for native notebooks on/after 1st August (#12961) (#12975) Co-authored-by: Joyce Er <[email protected]> Co-authored-by: Joyce Er <[email protected]> * Contains cherry picks, version updates, change log updates (#12983) * Update version and change log * Improve detection when LS is fully loaded for IntelliCode (#12853) * Fix path * Actually fix settings * Add news * Add test * Format * Suppress 'jediEnabled' removal * Drop survey first launch threshold * Wait for client ready * Handle async dispose * Fix the date Co-authored-by: Mikhail Arkhipov <[email protected]> * hide the gather button while a cell is executing (#12984) * Update date (#13002) * remove release notes from the start page (#13032) * Cherry pick, version change and change log update (#13079) * Ensure languageServer value is valid, send event during activate (#13064) * Update change log and version * Activate banner prompt for Pylance (#12817) * Fix path * Actually fix settings * Add news * Add test * Format * Suppress 'jediEnabled' removal * Drop survey first launch threshold * Remove LS experiments * Frequency + tests * Fix test * Update message to match spec * Open workspace for extension rather than changing setting * Fix localization string * Show banners asynchronously * Add experiments * Formatting * Typo * Put back verifyAll * Remove obsolete experiments, add Pylance * Suppress experiment if Pylance is installed * PR feedback Co-authored-by: Jake Bailey <[email protected]> * Update change log as per comments Co-authored-by: Jake Bailey <[email protected]> Co-authored-by: Mikhail Arkhipov <[email protected]> * Port fix the gather survey (#13086) (#13105) * Fix the gather survey (#13086) * fix the gather survey added 'gather stats' telemetry mention the gather comments to update the python ext * oops * fix tests and address comments * update gather stats when resetting the kernel * set globalstate vars to 0 when we open vs code * fix gather stats telemetry * fix tests * fix tests for real Co-authored-by: Joyce Er <[email protected]> Co-authored-by: Ian Huff <[email protected]> Co-authored-by: David Kutugata <[email protected]> Co-authored-by: Don Jayamanne <[email protected]> Co-authored-by: Timothy Ruscica <[email protected]> Co-authored-by: Mikhail Arkhipov <[email protected]> Co-authored-by: Jake Bailey <[email protected]>

fix the gather survey

c3393fb

added 'gather stats' telemetry mention the gather comments to update the python ext

DavidKutu added the no-changelog No news entry required label Jul 21, 2020

DavidKutu requested review from DonJayamanne, greazer, IanMatthewHuff, joyceerhl, rchiodo and techwithtim July 21, 2020 23:49

oops

8aed582

greazer suggested changes Jul 22, 2020

View reviewed changes

fix tests and address comments

744ee68

DavidKutu requested a review from greazer July 22, 2020 08:35

DavidKutu added 2 commits July 22, 2020 08:56

update gather stats when resetting the kernel

b9c22cc

set globalstate vars to 0 when we open vs code

f486d67

greazer approved these changes Jul 22, 2020

View reviewed changes

rchiodo reviewed Jul 22, 2020

View reviewed changes

rchiodo requested changes Jul 22, 2020

View reviewed changes

fix gather stats telemetry

e8a0a9a

DavidKutu requested a review from rchiodo July 22, 2020 20:32

rchiodo approved these changes Jul 22, 2020

View reviewed changes

DavidKutu merged commit 5d21a13 into master Jul 22, 2020

DavidKutu deleted the david/fixGatherSurvey branch July 22, 2020 21:54

Fix the gather survey #13086

Fix the gather survey #13086

Uh oh!

Conversation

DavidKutu commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jul 22, 2020

Uh oh!

Uh oh!

DavidKutu commented Jul 21, 2020 •

edited

Loading

codecov-commenter commented Jul 21, 2020 •

edited

Loading