Added telemetry for usage of activateEnvInCurrentTerminal setting #8654

karrtikr · 2019-11-19T00:16:28Z

The heart of the change is this file src/client/providers/terminalProvider.ts. I am just adding telemetry and passing in the active resource.

Other changes are related to #8654 (comment) which just involves refactoring the method getActiveResource().

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
Title summarizes what is changing
Has a news entry file (remember to thank yourself!)
Appropriate comments and documentation strings in the code
Has sufficient logging.
Has telemetry for enhancements.
Unit tests & system/integration tests are added/updated
~~[ ] Test plan is updated as appropriate~~
~~[ ] package-lock.json has been regenerated by running npm install (if dependencies have changed)~~
~~[ ] The wiki is updated with any design decisions/details.~~

codecov-io · 2019-11-19T00:50:40Z

Codecov Report

Merging #8654 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #8654      +/-   ##
==========================================
+ Coverage   59.18%   59.19%   +0.01%     
==========================================
  Files         521      522       +1     
  Lines       23977    23987      +10     
  Branches     3873     3870       -3     
==========================================
+ Hits        14190    14200      +10     
- Misses       8870     8871       +1     
+ Partials      917      916       -1

Impacted Files	Coverage Δ
src/client/telemetry/index.ts	`86.86% <ø> (ø)`	⬆️
src/client/common/application/activeResource.ts	`100% <100%> (ø)`
src/client/activation/serviceRegistry.ts	`100% <100%> (ø)`	⬆️
src/client/terminals/activation.ts	`82.75% <100%> (+3.97%)`	⬆️
src/client/providers/replProvider.ts	`100% <100%> (ø)`	⬆️
src/client/common/application/types.ts	`100% <100%> (ø)`	⬆️
src/client/providers/terminalProvider.ts	`100% <100%> (ø)`	⬆️
src/client/activation/activationManager.ts	`72.72% <100%> (+4.15%)`	⬆️
src/client/telemetry/constants.ts	`100% <100%> (ø)`	⬆️
src/client/testing/codeLenses/testFiles.ts	`17.43% <0%> (-0.67%)`	⬇️
... and 3 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 97d017c...201342e. Read the comment docs.

karthiknadig · 2019-11-19T07:10:57Z

src/test/providers/terminal.unit.test.ts

@@ -158,12 +160,15 @@ suite('Terminal Provider', () => {
        let configService: TypeMoq.IMock<IConfigurationService>;
        let terminalActivator: TypeMoq.IMock<ITerminalActivator>;
        let terminal: TypeMoq.IMock<Terminal>;
+        // tslint:disable-next-line:no-any
+        let getActiveResource: sinon.SinonStub<any>;


For my info, why sinon? feels like typemoq could achieve the same.

Because I am trying to mock the methods of the class I am testing itself.
Typemoq can be used to mock methods of dependencies of a class just fine, but not the methods of the class I am testing. (I don't have mocked objects of the sort TypeMoq.IMock<InterfaceName> for the class I am testing)

@ericsnowcurrently @karthiknadig

The idea of mocking out a method of the class against which I'm testing is a little mind-melting. There must be a simpler way.

ericsnowcurrently

mostly LGTM

My suggestions are all about formatting and wording in comments. :)

src/client/telemetry/index.ts

news/1 Enhancements/8004.md

src/test/providers/terminal.unit.test.ts

DonJayamanne

Telemetry is being captured in the wrong place, only when extension is opened! (Rejecting PR because of this).
Terminal name can contain PII, also we don't need this (not to my knowledge). - optional if others think it's not PII (there's no guarantee it's not)

src/client/providers/terminalProvider.ts

luabud · 2019-11-19T17:02:58Z

my bad folks, I should have clarified what kind of telemetry I was expecting. It's just to track usage (i.e. how many people have the setting set to true), just to help with decisions in the future in terms of maintenance. Let's drop the terminal name since we can't guarantee it won't ever collect PII.

DonJayamanne · 2019-11-19T17:19:09Z

(i.e. how many people have the setting set to true)

Then i'd track this feature usage as part of the extension load, instead of tracking activated terminals when they are opened, etc.

karrtikr · 2019-11-19T19:58:45Z

I changed telemetry after consulting with @luabud . We want to track the usage of the setting (if it's present or not) and also the times when the setting is actually used to activate the terminal.

src/client/activation/activeResource.ts

Co-Authored-By: Eric Snow <ericsnowcurrently@gmail.com>

DonJayamanne

I'd still like to see the PR changed to tracking the setting as opposed to its usage.
It could be used elsewhere in the future.
This way its obvious, right now we're tracking a feature and not a setting. Hence looking at the code the telemetry isn't obvious.

ericsnowcurrently

The actual fix LGTM.

However, I have a bunch of comments related to the effort to factor out getting the "active" resource. I strongly recommend moving that to a separate PR.

ericsnowcurrently · 2019-11-20T16:27:30Z

src/client/activation/types.ts

@@ -169,3 +169,8 @@ export const IExtensionSingleActivationService = Symbol('IExtensionSingleActivat
 export interface IExtensionSingleActivationService {
    activate(): Promise<void>;
 }
+
+export const IActiveResourceService = Symbol('IActiveResourceService');
+export interface IActiveResourceService {


Please add a doc comment explaining the purpose of this type.

Yes. Though I don't think we even need an interface for this.
We could just create the class and inject just the class.

@DonJayamanne For the cases where serviceContainer is used,

this.activeResourceService = this.serviceContainer.get<IActiveResourceService>(IActiveResourceService);

we can't inject the class, can we? How do we initialize this.activeResourceService in these cases?

src/client/providers/terminalProvider.ts

src/client/activation/activeResource.ts

src/test/activation/activeResource.unit.test.ts

src/client/activation/activeResource.ts

ericsnowcurrently · 2019-11-20T18:17:38Z

src/test/activation/activeResource.unit.test.ts

+    });
+
+    test('Return document uri if a saved document is currently opened', async () => {
+        const activeTextEditor = {


For a unit test I would expect use of mocks for both activeTextEditor and activeTextEditor.document. Then you would verify that isUntitled and uri were both used and that nothing else was. So I suppose the test would look more like this:

test('Return document uri if a saved document is currently opened', async () => { const expected = Uri.parse('a'); const activeTextEditor: TextEditor = mock(TextEditor); const document: TextDocument = mock(TextDocument); when(document.isUntitled).thenReturn(false); when(document.uri).thenReturn(expected); when(activeTextEditor.document).thenReturn(document); when(documentManager.activeTextEditor).thenReturn(activeTextEditor); //when(workspaceService.workspaceFolders).thenReturn([]); const activeResource = activeResourceService.getActiveResource(); assert.deepEqual(activeResource, expected); verify(document.isUntitled).once(); verify(document.uri).once(); verify(activeTextEditor.document).once(); verify(documentManager.activeTextEditor).atLeast(1); verify(workspaceService.workspaceFolders).never(); });

The trick is ensuring that you verify everything properly. That is a lot easier with TypeMoq:

suite('Active resource service', () => { let document: TypeMoq.IMock<vscode.TextDocument>; let editor: TypeMoq.IMock<vscode.TextEditor>; let documentManager: TypeMoq.IMock<IDocumentManager>; let workspaceService: TypeMoq.IMock<IWorkspaceService>; let activeResourceService: ActiveResourceService; setup(() => { document = TypeMoq.Mock.ofType<vscode.TextDocument>(undefined, TypeMoq.MockBehavior.Strict); editor = TypeMoq.Mock.ofType<vscode.TextEditor>(undefined, TypeMoq.MockBehavior.Strict); documentManager = TypeMoq.Mock.ofType<IDocumentManager>(undefined, TypeMoq.MockBehavior.Strict); workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>(undefined, TypeMoq.MockBehavior.Strict); activeResourceService = new ActiveResourceService( documentManager.object, workspaceService.object ); }); function verifyAll() { document.verifyAll(); editor.verifyAll(); documentManager.verifyAll(); workspaceService.verifyAll(); } test('Return document uri if a saved document is currently opened', async () => { const expected = Uri.parse('a'); documentManager.setup(dm => dm.activeTextEditor) .returns(() => editor); editor.setup(e => e.document) .returns(() => document); document.setup(d => d.isUntitled) .returns(() => false); document.setup(d => d.uri) .returns(() => expected); const activeResource = activeResourceService.getActiveResource(); assert.deepEqual(activeResource, expected); verifyAll(); });

I still like ts-mokito :) and the old test was good enough for me.

I'm just worried that we aren't checking for unexpected behavior (spurious calls). Does ts-mokito offer a way to verify that only the calls you expect actually happened?

We can do the same in ts-mokito, but I am thinking that the old test was good enough.
Do we really need to verify every little aspect? Seems a bit too much.

@ericsnowcurrently I have some concerns that verifying every count can lead to high coupling. Until we have a team discussion regarding best testing practices, I would go with the existing approach. Please approve the PR if you find everything else okay.

That's fine. :)

luabud · 2019-11-20T18:35:36Z

@DonJayamanne

I'd still like to see the PR changed to tracking the setting as opposed to its usage.
It could be used elsewhere in the future.
This way its obvious, right now we're tracking a feature and not a setting. Hence looking at the code the telemetry isn't obvious.

I was aiming to track usage of the feature for the purpose of future decision making in terms of maintenance and feature work. I understand that if we change the behaviour of the feature and/or setting, we'll probably need to change the telemetry as well, but I don't think we're saving up so much time by tracking something else now. Of course we should always aim for extensible solutions but in this case it's telemetry being captured, not feature or bug fixing work. Does that make sense?

DonJayamanne · 2019-11-20T19:08:15Z

I'd still like to see the PR changed to tracking the setting as opposed to its usage.

@luabud not sure what u mean.
Here's your requirement (i.e. how many people have the setting set to true),
So my comments were based on the above requirement.

src/client/providers/terminalProvider.ts

DonJayamanne · 2019-11-20T20:02:44Z

The original requirement was (i.e. how many people have the setting set to true)
But now we're doing something else.
We're not tracking how many have the setting on, were tracking how many are using it... Different meaning.

Ie there's a disconnect between the requirement and the solution implementation..

karrtikr · 2019-11-20T20:07:46Z

@DonJayamanne We're tracking both, Don. Please have a look at my earlier comment. The telemetry event is sent only if the setting is on, and even if the user is not actually using the setting.

Co-Authored-By: Eric Snow <ericsnowcurrently@gmail.com>

Just waiting for your review

ericsnowcurrently

LGTM

Honestly, this would have landed a lot sooner if the ActiveResourceService stuff had been done in a separate PR. :)

karrtikr requested review from ericsnowcurrently, karthiknadig and kimadeline November 19, 2019 00:17

karthiknadig reviewed Nov 19, 2019

View reviewed changes

ericsnowcurrently suggested changes Nov 19, 2019

View reviewed changes

DonJayamanne requested changes Nov 19, 2019

View reviewed changes

src/client/providers/terminalProvider.ts Outdated Show resolved Hide resolved

src/client/providers/terminalProvider.ts Outdated Show resolved Hide resolved

DonJayamanne reviewed Nov 19, 2019

View reviewed changes

src/client/activation/activeResource.ts Outdated Show resolved Hide resolved

karrtikr force-pushed the activateEnvInCurrentTerminal branch from f70b382 to 85755b1 Compare November 19, 2019 22:24

Kartik Raj and others added 8 commits November 19, 2019 16:07

Added telemetry and tests

4667af1

News entry

30fd058

Apply suggestions from code review

6bb8fc1

Co-Authored-By: Eric Snow <ericsnowcurrently@gmail.com>

Modified telemetry + code reviews

d3a448f

Oops

8e41eea

Added tests

bc3df8a

Move getActiveResource to a common place

e8304f3

Refactor more stuff

1e4b232

karrtikr force-pushed the activateEnvInCurrentTerminal branch from eb9ad68 to 1e4b232 Compare November 20, 2019 00:08

karrtikr requested review from DonJayamanne, karthiknadig and ericsnowcurrently and removed request for kimadeline November 20, 2019 00:27

karrtikr closed this Nov 20, 2019

karrtikr reopened this Nov 20, 2019

karrtikr closed this Nov 20, 2019

karrtikr reopened this Nov 20, 2019

DonJayamanne requested changes Nov 20, 2019

View reviewed changes

ericsnowcurrently previously requested changes Nov 20, 2019

View reviewed changes

karrtikr commented Nov 20, 2019

View reviewed changes

src/client/providers/terminalProvider.ts Show resolved Hide resolved

Moved into common/application

754d2d8

DonJayamanne approved these changes Nov 20, 2019

View reviewed changes

Kartik Raj and others added 5 commits November 20, 2019 13:36

Apply suggestions from code review

0b3b802

Co-Authored-By: Eric Snow <ericsnowcurrently@gmail.com>

Update src/test/activation/activeResource.unit.test.ts

a0dfc5c

Co-Authored-By: Eric Snow <ericsnowcurrently@gmail.com>

Code reviews

39509dc

Don't overuse tslint any

66fd30c

Add description

7b00729

karrtikr requested a review from ericsnowcurrently November 20, 2019 23:29

Better format

201342e

ericsnowcurrently approved these changes Nov 25, 2019

View reviewed changes

karrtikr merged commit 68ef910 into microsoft:master Nov 25, 2019

karrtikr deleted the activateEnvInCurrentTerminal branch November 25, 2019 18:31

lock bot locked as resolved and limited conversation to collaborators Dec 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added telemetry for usage of activateEnvInCurrentTerminal setting #8654

Added telemetry for usage of activateEnvInCurrentTerminal setting #8654

karrtikr commented Nov 19, 2019 •

edited

Loading

codecov-io commented Nov 19, 2019 •

edited

Loading

karthiknadig Nov 19, 2019

karrtikr Nov 19, 2019

ericsnowcurrently Nov 20, 2019

ericsnowcurrently left a comment •

edited

Loading

DonJayamanne left a comment

luabud commented Nov 19, 2019

DonJayamanne commented Nov 19, 2019

karrtikr commented Nov 19, 2019

DonJayamanne left a comment

ericsnowcurrently left a comment

ericsnowcurrently Nov 20, 2019

DonJayamanne Nov 20, 2019

karrtikr Nov 20, 2019 •

edited

Loading

ericsnowcurrently Nov 20, 2019

DonJayamanne Nov 20, 2019

ericsnowcurrently Nov 20, 2019

karrtikr Nov 20, 2019

karrtikr Nov 21, 2019

ericsnowcurrently Nov 25, 2019

luabud commented Nov 20, 2019

DonJayamanne commented Nov 20, 2019

DonJayamanne commented Nov 20, 2019 •

edited

Loading

karrtikr commented Nov 20, 2019

ericsnowcurrently left a comment

Added telemetry for usage of activateEnvInCurrentTerminal setting #8654

Added telemetry for usage of activateEnvInCurrentTerminal setting #8654

Conversation

karrtikr commented Nov 19, 2019 • edited Loading

codecov-io commented Nov 19, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsnowcurrently left a comment • edited Loading

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

luabud commented Nov 19, 2019

DonJayamanne commented Nov 19, 2019

karrtikr commented Nov 19, 2019

DonJayamanne left a comment

Choose a reason for hiding this comment

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karrtikr Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luabud commented Nov 20, 2019

DonJayamanne commented Nov 20, 2019

DonJayamanne commented Nov 20, 2019 • edited Loading

karrtikr commented Nov 20, 2019

ericsnowcurrently left a comment

Choose a reason for hiding this comment

karrtikr commented Nov 19, 2019 •

edited

Loading

codecov-io commented Nov 19, 2019 •

edited

Loading

ericsnowcurrently left a comment •

edited

Loading

karrtikr Nov 20, 2019 •

edited

Loading

DonJayamanne commented Nov 20, 2019 •

edited

Loading