PR feedback

Author: Paul van Brenk

Nodejs/Product/Nodejs/NpmUI/NpmPackageInstallViewModel.cs

@@ -304,7 +304,7 @@ internal void Install(PackageCatalogEntryViewModel package)
             {
                 var selectedVersion = this.SelectedVersion is SemverVersion ? ((SemverVersion)this.SelectedVersion).ToString() : string.Empty;
 
-                TelemetryHelper.LogInstallNpmPackage(package.Name, selectedVersion);
+                TelemetryHelper.LogInstallNpmPackage();
 
                 this.npmWorker.QueueCommand(
                     NpmArgumentBuilder.GetNpmInstallArguments(

Nodejs/Product/Nodejs/NpmUI/NpmWorker.cs

@@ -104,7 +104,7 @@ public async Task<IEnumerable<IPackage>> GetCatalogPackagesAsync(string filterTe
                 return Enumerable.Empty<IPackage>();
             }
 
-            TelemetryHelper.LogSearchNpm(filterText);
+            TelemetryHelper.LogSearchNpm();
 
             var relativeUri = string.Format("/-/v1/search?text={0}", WebUtility.UrlEncode(filterText));
             var searchUri = new Uri(defaultRegistryUri, relativeUri);

Nodejs/Product/Nodejs/Project/NodeModulesNode.cs

@@ -621,7 +621,7 @@ public System.Threading.Tasks.Task UninstallModules()
             {
                 foreach (var node in selected.OfType<DependencyNode>().Where(this.CheckValidCommandTarget))
                 {
-                    TelemetryHelper.LogUnInstallNpmPackage(node.Package.Name);
+                    TelemetryHelper.LogUnInstallNpmPackage();
                     await commander.UninstallPackageAsync(node.Package.Name);
                 }
             });
@@ -634,7 +634,7 @@ public async System.Threading.Tasks.Task UninstallModule(DependencyNode node)
                 return;
             }
 
-            TelemetryHelper.LogUnInstallNpmPackage(node.Package.Name);
+            TelemetryHelper.LogUnInstallNpmPackage();
 
             await RunNpmCommand(async commander =>
             {

Nodejs/Product/Nodejs/Project/NodejsProjectLauncher.cs

@@ -76,6 +76,7 @@ private int Start(string file, bool debug)
             var chromeProtocolRequired = nodeVersion >= new Version(8, 0) || CheckDebugProtocolOption();
             var startBrowser = ShouldStartBrowser();
 
+            // The call to Version.ToString() is safe, since changes to the ToString method are very unlikely, as the current output is widely documented.
             if (debug && !chromeProtocolRequired)
             {
                 StartWithDebugger(file);

Nodejs/Product/Nodejs/Telemetry/TelemetryEvents.cs

@@ -62,20 +62,5 @@ internal static class TelemetryProperties
         /// The version of Node the user is using.
         /// </summary>
         public const string NodeVersion = Prefix + "NodeVersion";
-
-        /// <summary>
-        /// The query the user send to NPM.
-        /// </summary>
-        public const string NpmSearchQuery = Prefix + "NpmSearchQuery";
-
-        /// <summary>
-        /// The NPM package being installed/uninstalled/updated.
-        /// </summary>
-        public const string NpmPackageName = Prefix + "NpmPackageName";
-
-        /// <summary>
-        /// The version of the NPM package being installed/uninstalled/updated.
-        /// </summary>
-        public const string NpmPackageVersion = Prefix + "NpmPackageVersion";
     }
 }

Nodejs/Product/Nodejs/Telemetry/TelemetryHelper.cs

@@ -37,32 +37,22 @@ public static void LogDebuggingStarted(string debuggerName, string nodeVersion,
             defaultSession.PostEvent(userTask);
         }
 
-        public static void LogSearchNpm(string query)
+        public static void LogSearchNpm()
         {
-            var userTask = new UserTaskEvent(SearchNpm, TelemetryResult.Success);
-            userTask.Properties[NpmSearchQuery] = query;
-
-            defaultSession.PostEvent(userTask);
+            defaultSession.PostUserTask(SearchNpm, TelemetryResult.Success);
         }
 
-        public static void LogInstallNpmPackage(string packageName, string version)
+        public static void LogInstallNpmPackage()
         {
-            var userTask = new UserTaskEvent(InstallNpm, TelemetryResult.Success);
-            userTask.Properties[NpmPackageName] = packageName;
-            userTask.Properties[NpmPackageVersion] = packageName;
-
-            defaultSession.PostEvent(userTask);
+            defaultSession.PostUserTask(InstallNpm, TelemetryResult.Success);
         }
 
-        public static void LogUnInstallNpmPackage(string packageName)
+        public static void LogUnInstallNpmPackage()
         {
-            var userTask = new UserTaskEvent(UnInstallNpm, TelemetryResult.Success);
-            userTask.Properties[NpmPackageName] = packageName;
-
-            defaultSession.PostEvent(userTask);
+            defaultSession.PostUserTask(UnInstallNpm, TelemetryResult.Success);
         }
 
-        internal static void LogReplUse()
+        public static void LogReplUse()
         {
             defaultSession.PostUserTask(UsedRepl, TelemetryResult.Success);
         }

Nodejs/Product/TestAdapter/TestExecutor.cs

@@ -183,12 +183,16 @@ public void RunTests(IEnumerable<TestCase> tests, IRunContext runContext, IFrame
             }
         }
 
-        private void LogTelemetry(int testCount, bool isDebugging)
+        private void LogTelemetry(int testCount, Version nodeVersion, bool isDebugging)
         {
             var userTask = new UserTaskEvent("VS/NodejsTools/UnitTestsExecuted", TelemetryResult.Success);
             userTask.Properties["VS.NodejsTools.TestCount"] = testCount;
+            // This is safe, since changes to the ToString method are very unlikely, as the current output is widely documented.
+            userTask.Properties["VS.NodejsTools.NodeVersion"] = nodeVersion.ToString(); 
             userTask.Properties["VS.NodejsTools.IsDebugging"] = isDebugging;
 
+            //todo: when we have support for the Node 8 debugger log which version of the debugger people are actually using
+
             var defaultSession = TelemetryService.DefaultSession;
             defaultSession.PostEvent(userTask);
         }
@@ -221,12 +225,14 @@ private void RunTestCases(IEnumerable<TestCase> tests, IRunContext runContext, I
                 var testInfo = new NodejsTestInfo(tests.First().FullyQualifiedName);
                 var workingDir = Path.GetDirectoryName(CommonUtils.GetAbsoluteFilePath(settings.WorkingDir, testInfo.ModulePath));
 
-                // we can only log telemetry when we're running in VS,
-                // the required assemblies are not on disk, so we have to keep the call in a separate method
-                // this way the .NET framework only loads the assemblies when we actually need them.
+                var nodeVersion = Nodejs.GetNodeVersion(settings.NodeExePath);
+
+                // We can only log telemetry when we're running in VS.
+                // Since the required assemblies are not on disk if we're not running in VS, we have to reference them in a separate method
+                // this way the .NET framework only tries to load the assemblies when we actually need them.
                 if (app != null)
                 {
-                    LogTelemetry(tests.Count(), runContext.IsBeingDebugged);
+                    LogTelemetry(tests.Count(), nodeVersion, runContext.IsBeingDebugged);
                 }
 
                 foreach (var test in tests)