Merge pull request #806 from github-for-unity/fixes/metrics-shouldnt-be-null

Usage tracker should never be null

Author: StanleyGoldman

src/GitHub.Api/Application/ApplicationManagerBase.cs

@@ -47,6 +47,16 @@ protected void Initialize()
             ApplicationConfiguration.WebTimeout = UserSettings.Get(Constants.WebTimeoutKey, ApplicationConfiguration.WebTimeout);
             Platform.Initialize(ProcessManager, TaskManager);
             progress.OnProgress += progressReporter.UpdateProgress;
+            UsageTracker = new UsageTracker(UserSettings, Environment, InstanceId.ToString());
+
+#if ENABLE_METRICS
+            var metricsService = new MetricsService(ProcessManager,
+                TaskManager,
+                Environment.FileSystem,
+                Environment.NodeJsExecutablePath,
+                Environment.OctorunScriptPath);
+            UsageTracker.MetricsService = metricsService;
+#endif
         }
 
         public void Run()
@@ -59,8 +69,7 @@ public void Run()
                 GitInstallationState state = new GitInstallationState();
                 try
                 {
-                    SetupMetrics(Environment.UnityVersion);
-
+                    SetupMetrics();
                     if (Environment.IsMac)
                     {
                         var getEnvPath = new SimpleProcessTask(TaskManager.Token, "bash".ToNPath(), "-c \"/usr/libexec/path_helper\"")
@@ -301,35 +310,14 @@ public void RestartRepository()
             Logger.Trace($"Got a repository? {(Environment.Repository != null ? Environment.Repository.LocalPath : "null")}");
         }
 
-        protected void SetupMetrics(string unityVersion)
+        protected void SetupMetrics()
         {
-            string userId = null;
-            if (UserSettings.Exists(Constants.GuidKey))
-            {
-                userId = UserSettings.Get(Constants.GuidKey);
-            }
-
-            if (String.IsNullOrEmpty(userId))
-            {
-                userId = Guid.NewGuid().ToString();
-                UserSettings.Set(Constants.GuidKey, userId);
-            }
-
-#if ENABLE_METRICS
-            var metricsService = new MetricsService(ProcessManager,
-                TaskManager,
-                Environment.FileSystem,
-                Environment.NodeJsExecutablePath,
-                Environment.OctorunScriptPath);
-
-            UsageTracker = new UsageTracker(metricsService, UserSettings, Environment, userId, unityVersion, InstanceId.ToString());
-
             if (firstRun)
             {
                 UsageTracker.IncrementNumberOfStartups();
             }
-#endif
         }
+
         protected abstract void InitializeUI();
         protected abstract void InitializationComplete();
 

src/GitHub.Api/Metrics/IMetricsService.cs

@@ -3,7 +3,7 @@
 
 namespace GitHub.Unity
 {
-    interface IMetricsService
+    public interface IMetricsService
     {
         /// <summary>
         /// Posts the provided usage model.

src/GitHub.Api/Metrics/IUsageTracker.cs

@@ -3,6 +3,7 @@
     public interface IUsageTracker
     {
         bool Enabled { get; set; }
+        IMetricsService MetricsService { get; set; }
         void IncrementNumberOfStartups();
         void IncrementChangesViewButtonCommit();
         void IncrementHistoryViewToolbarFetch();

src/GitHub.Api/Metrics/UsageTracker.cs

@@ -14,33 +14,43 @@ class UsageTracker : IUsageTracker
 
         private readonly ISettings userSettings;
         private readonly IUsageLoader usageLoader;
-        private readonly IMetricsService metricsService;
         private readonly string userId;
         private readonly string appVersion;
         private readonly string unityVersion;
         private readonly string instanceId;
         private Timer timer;
 
-        public UsageTracker(IMetricsService metricsService, ISettings userSettings,
-            IEnvironment environment, string userId, string unityVersion, string instanceId)
-                : this(metricsService, userSettings, 
-                      new UsageLoader(environment.UserCachePath.Combine(Constants.UsageFile)),
-                      userId, unityVersion, instanceId)
+        public IMetricsService MetricsService { get; set; }
+
+        public UsageTracker(ISettings userSettings,
+            IEnvironment environment, string instanceId)
+                : this(userSettings, 
+                    new UsageLoader(environment.UserCachePath.Combine(Constants.UsageFile)),
+                    environment.UnityVersion, instanceId)
         {
         }
 
-        public UsageTracker(IMetricsService metricsService, ISettings userSettings,
+        public UsageTracker(ISettings userSettings,
             IUsageLoader usageLoader,
-            string userId, string unityVersion, string instanceId)
+            string unityVersion, string instanceId)
         {
             this.userSettings = userSettings;
             this.usageLoader = usageLoader;
-            this.metricsService = metricsService;
-            this.userId = userId;
             this.appVersion = ApplicationInfo.Version;
             this.unityVersion = unityVersion;
             this.instanceId = instanceId;
 
+            if (userSettings.Exists(Constants.GuidKey))
+            {
+                userId = userSettings.Get(Constants.GuidKey);
+            }
+
+            if (String.IsNullOrEmpty(userId))
+            {
+                userId = Guid.NewGuid().ToString();
+                userSettings.Set(Constants.GuidKey, userId);
+            }
+
             Logger.Trace("userId:{0} instanceId:{1}", userId, instanceId);
             if (Enabled)
                 RunTimer(3*60);
@@ -61,14 +71,14 @@ private void RunTimer(int seconds)
 
         private async Task SendUsage()
         {
-            var usageStore = usageLoader.Load(userId);
-
-            if (metricsService == null)
+            if (MetricsService == null)
             {
                 Logger.Warning("No service, not sending usage");
                 return;
             }
 
+            var usageStore = usageLoader.Load(userId);
+
             if (usageStore.LastUpdated.Date != DateTimeOffset.UtcNow.Date)
             {
                 var currentTimeOffset = DateTimeOffset.UtcNow;
@@ -90,7 +100,7 @@ private async Task SendUsage()
 
                     try
                     {
-                        await metricsService.PostUsage(extractReports);
+                        await MetricsService.PostUsage(extractReports);
                         success = true;
                     }
                     catch (Exception ex)

src/tests/IntegrationTests/Metrics/MetricsTests.cs

@@ -34,11 +34,14 @@ public void IncrementMetricsWorks(string measureName)
             var instanceId = Guid.NewGuid().ToString();
             var usageLoader = Substitute.For<IUsageLoader>();
             var usageStore = new UsageStore();
+            var settings = Substitute.For<ISettings>();
+            settings.Exists(Arg.Is<string>(Constants.GuidKey)).Returns(true);
+            settings.Get(Arg.Is<string>(Constants.GuidKey)).Returns(userId);
+
             usageStore.Model.Guid = userId;
             usageLoader.Load(Arg.Is<string>(userId)).Returns(usageStore);
 
-            var usageTracker = new UsageTracker(Substitute.For<IMetricsService>(), Substitute.For<ISettings>(),
-                usageLoader, userId, unityVersion, instanceId);
+            var usageTracker = new UsageTracker(settings, usageLoader, unityVersion, instanceId);
 
             var currentUsage = usageStore.GetCurrentMeasures(appVersion, unityVersion, instanceId);
             var prop = currentUsage.GetType().GetProperty(measureName);
@@ -59,14 +62,21 @@ public void LoadingWorks()
             var instanceId = Guid.NewGuid().ToString();
             var usageStore = new UsageStore();
             usageStore.Model.Guid = userId;
-            var usageTracker = new UsageTracker(Substitute.For<IMetricsService>(), Substitute.For<ISettings>(),
-                Environment, userId, unityVersion, instanceId);
-            usageTracker.IncrementNumberOfStartups();
             var storePath = Environment.UserCachePath.Combine(Constants.UsageFile);
+            var usageLoader = new UsageLoader(storePath);
+
+            var settings = Substitute.For<ISettings>();
+            settings.Exists(Arg.Is<string>(Constants.GuidKey)).Returns(true);
+            settings.Get(Arg.Is<string>(Constants.GuidKey)).Returns(userId);
+            var usageTracker = new UsageTracker(settings, usageLoader, unityVersion, instanceId);
+
+            usageTracker.IncrementNumberOfStartups();
+            usageTracker.IncrementNumberOfStartups();
+
             Assert.IsTrue(storePath.FileExists());
             var json = storePath.ReadAllText(Encoding.UTF8);
             var savedStore = json.FromJson<UsageStore>(lowerCase: true);
-            Assert.AreEqual(1, savedStore.GetCurrentMeasures(appVersion, unityVersion, instanceId).NumberOfStartups);
+            Assert.AreEqual(2, savedStore.GetCurrentMeasures(appVersion, unityVersion, instanceId).NumberOfStartups);
         }
     }
 }