Make it easier to add new usage metrics. #1130

grokys · 2017-08-10T14:30:49Z

Currently, adding a new usage metric is a bit of a PITA. You need to:

Add a new field to UsageModel
Add a new method to IUsageTracker
Add a new method to UsageTrackerDispatcher
Add a new method to UsageTracker
Add the field to UsageTracker.ClearCounters
Add the field to UsageModel.Clone

This PR:

Removes the individual IncrementX methods from IUsageTracker and replaces them with an IncrementCounter method which takes a lambda describing the metric to update.
Uses reflection to reset the counters in UsageTracker.ClearCounters
Uses reflection to clone the counters in UsageModel.Clone
Moves IO and date comparison operations out of UsageTracker so it can be unit tested
Adds unit tests for UsageTracker

- Separated IO and date/time comparisons into a separate `UsageService` to allow for easier unit testing - Added some unit tests.

Instead of having a separate method for each usage metric, use a generialized method that takes an expression describing the metric to increment.

Conflicts: src/GitHub.App/Models/RepositoryHost.cs src/GitHub.VisualStudio/source.extension.vsixmanifest src/MsiInstaller/Version.wxi src/common/SolutionInfo.cs

jcansdale · 2017-09-06T15:14:19Z

src/GitHub.VisualStudio/Services/UsageService.cs

+
+        async Task WriteAllTextAsync(string path, string text)
+        {
+            using (var s = File.OpenWrite(path))


We should be creating a new file, not overwriting the old one.

We need to truncate if file already exists.

shana

Got a few questions and comments. Didn't go through all of it, so there might be more(tm) 😄
WRT splitting out IO so we can unit test, Rothko provides services just for that, fyi

shana · 2017-09-07T15:38:04Z

src/GitHub.VisualStudio/GitHub.VisualStudio.csproj

@@ -11,6 +11,7 @@
    <NuGetPackageImportStamp>
    </NuGetPackageImportStamp>
    <TargetFrameworkProfile />
+    <ApplicationVersion>2.3.1.0</ApplicationVersion>


This change shouldn't be in here. Bad merge somewhere?

Yep, bad merge - well spotted!

shana · 2017-09-07T15:39:56Z

src/GitHub.VisualStudio/Services/UsageService.cs

+
+        async Task<string> ReadAllTextAsync(string path)
+        {
+            using (var s = File.OpenRead(path))


You can use rothko for IO services that can be mocked for unit testing

Already thought of that, but Rothko doesn't have the methods we need as far as I can tell, which is partly why I split this I/O functionality out into a separate service apart from the usage tracker itself.

shana · 2017-09-07T15:43:31Z

src/GitHub.VisualStudio/Services/UsageTracker.cs

        async Task TimerTick()
        {
            await Initialize();

-            if (firstRun)
+            if (firstTick)
            {
                await IncrementLaunchCount();


IncrementLaunchCount calls LoadUsage which calls await Initialize which has already been called right before this. Do we need these double await Initialize calls?

Actually, yes we can remove the Initialize at the top of TimerTick! 👍

shana · 2017-09-07T15:44:17Z

src/GitHub.VisualStudio/Services/UsageTracker.cs

-                    Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData),
-                    program.ApplicationName,
-                    StoreFileName);
+                initialized = true;


Is there a possibility of a race condition here where await Initialize gets called at the same time?

There is a small possibility, if a counter is initialized on a background thread and while switching to the main thread, another counter is initialized. However all that would happen in that scenario is that services would get loaded twice, which I didn't think was a big enough problem to warrant putting a SemaphoreSlim or similar around.

The first tick will call `IncrementLaunchCount` which will call `LoadUsage` which will call `Initialize` for us.

grokys · 2017-11-08T14:42:23Z

@shana any chance of another review of this? I seem to remember you expressing surprise that this hadn't already gone in a few weeks back.

grokys added 3 commits August 10, 2017 14:49

Bump version to 2.3.1.0

49ba6cf

Added UsageTracker unit tests.

b419c1c

- Separated IO and date/time comparisons into a separate `UsageService` to allow for easier unit testing - Added some unit tests.

Added IUsageTracker.IncrementCounter

91d86ab

Instead of having a separate method for each usage metric, use a generialized method that takes an expression describing the metric to increment.

grokys force-pushed the refactor/usage-tracker branch from 9291c30 to 91d86ab Compare August 10, 2017 14:36

grokys added 2 commits August 10, 2017 17:22

GetService must called be on main thread.

1ef04a4

Use reflection to clone UsageModel.

7b1a37f

shana force-pushed the master branch from 49ba6cf to a82bce9 Compare August 16, 2017 12:54

Merge branch 'master' into refactor/usage-tracker

b6050c9

Conflicts: src/GitHub.App/Models/RepositoryHost.cs src/GitHub.VisualStudio/source.extension.vsixmanifest src/MsiInstaller/Version.wxi src/common/SolutionInfo.cs

grokys changed the title ~~WIP: Make it easier to add new usage metrics.~~ Make it easier to add new usage metrics. Sep 6, 2017

jcansdale reviewed Sep 6, 2017

View reviewed changes

Don't overwrite existing file contents.

a8acf7e

We need to truncate if file already exists.

jcansdale mentioned this pull request Sep 7, 2017

Enable assembly resolver to ensure that Start Page and other icons can be found #1220

Merged

shana suggested changes Sep 7, 2017

View reviewed changes

grokys added 2 commits September 7, 2017 18:52

Fix bad merge.

5876ed4

Don't initialize at the top of TimerTick.

15802f5

The first tick will call `IncrementLaunchCount` which will call `LoadUsage` which will call `Initialize` for us.

grokys mentioned this pull request Oct 12, 2017

Functionality to easily add new metrics #1267

Merged

Merge branch 'master' into refactor/usage-tracker

4210dfb

Merge branch 'master' into refactor/usage-tracker

d6a1e42

shana approved these changes Nov 17, 2017

View reviewed changes

shana merged commit 6c852c2 into master Nov 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make it easier to add new usage metrics. #1130

Make it easier to add new usage metrics. #1130

Uh oh!

grokys commented Aug 10, 2017 •

edited

Loading

Uh oh!

jcansdale Sep 6, 2017

Uh oh!

shana left a comment

Uh oh!

shana Sep 7, 2017

Uh oh!

grokys Sep 7, 2017

Uh oh!

shana Sep 7, 2017

Uh oh!

grokys Sep 7, 2017

Uh oh!

shana Sep 7, 2017

Uh oh!

grokys Sep 7, 2017

Uh oh!

shana Sep 7, 2017

Uh oh!

grokys Sep 7, 2017

Uh oh!

grokys commented Nov 8, 2017

Uh oh!

Uh oh!

Make it easier to add new usage metrics. #1130

Make it easier to add new usage metrics. #1130

Uh oh!

Conversation

grokys commented Aug 10, 2017 • 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!

shana left a comment

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!

grokys commented Nov 8, 2017

Uh oh!

Uh oh!

grokys commented Aug 10, 2017 •

edited

Loading