Understand PMC and Powershell usages #3755

erdembayar · 2020-11-10T00:53:08Z

Bug

Fixes: Nuget/Home#10142
Child of https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1167790.
Regression: No

Last working version:
How are we preventing it in future:

Fix

Details:
Recently decided we not to act on decrease PMC usage issue. We suspect it's highly unlikely problem to happen. To better understand this problem we add those 3 types of telemetries.
Design at Nuget/Client.Engineering#582

@nkolev92 @zivkan @dominoFire please review.

If you might find choice names of telemetry properties could be not good then please comment so I can rename them.

vs/nuget/powershellloaded: This telemetry is for reporting that Powershell just got loaded, and we have flag indicated what triggered powershell load.
This one is important because we can detect if VS crash before we emit other PowerShell usage telemetries.

Property	Desc
vs.nuget.trigger	PMUI or PMC

vs/nuget/solutionclose: This telemetry is for PMC/PMUI usage during VS solution session.

Property	Desciption	Value1	Value2
vs.nuget.powershellhost.firsttimeloadedfrompmc	If PMC PowerShellHost get loaded then was it first time during this VS instance session?	true = Yes	false = No
vs.nuget.powershellhost.firsttimeloadedfrompmui	If PMUI PowerShellHost get loaded then was it first time during this VS instance session?	true = Yes	false = No
vs.nuget.powershellhost.initps1loadedfrompmcfirst	Which one was first from below 2 events?	true = PMC	false = PMUI
vs.nuget.powershellhost.initps1loadpmc	Did PMC execute init.ps1 during current VS solution session ?	true = Yes	false = No
vs.nuget.powershellhost.initps1loadpmui	Did PMUI execute init.ps1 during current VS solution session ?	true = Yes	false = No
vs.nuget.powershellhost.loadedfrompmc	Did PMC PowerShellHost load during current VS solution session?	true = Yes	false = No
vs.nuget.powershellhost.loadedfrompmui	Did PMUI PowerShellHost load during current VS solution session?	true = Yes	false = No
vs.nuget.powershellhost.nugetcommandused	Did user execute any Nuget command (e.g. install-package)?	true = Yes	false = No
vs.nuget.powershellhost.pmcexecutecommandcount	Number of commands executed on PMC
vs.nuget.powershellhost.pmcwindowloadcount	Number of times PMC window got user focus
vs.nuget.powershellhost.pmuiexecutecommandcount	Number of powershell commands executed fromPMUI
vs.nuget.powershellhost.solutionloaded	If solution was loaded? PMC window can be used without solution.	true = Yes	false = No

vs/nuget/instanceclose This telemetry is for VS instance close, so it has some aggregated data in it.
For example below circled one shows PMC window last focused window, therefore it'll be automatically re-opened when we open VS next time.

Property	Desciption	Value1	Value2
vs.nuget.powershellhost.pmcexecutecommandcount	Number of commands executed on PMC
vs.nuget.powershellhost.pmcpowershellloadedsolutioncount	Number of times PMC window got user focus
vs.nuget.powershellhost.pmcwindowloadcount	Number solution PMC powershell was loaded (May include PMC session without solution load).
vs.nuget.powershellhost.pmuiexecutecommandcount	If PMC window going to reopen	true = Yes	false = No
vs.nuget.powershellhost.reopenatstart	Number solution PMUI powershell was loaded.
vs.nuget.powershellhost.solutioncount	Number of actual solutions loaded

Testing/Validation

Tests Added: No
Reason for not adding tests: It's just telemetry, I may add later once current PR gets 1 approval.
Validation: Lots of manual validation.

Add telemetry for if PMC opened by default without user gesture. Add telemetry for how many times powershell commands executed.

…r PM UI and PM PMC. So make change to accommodiate this concern.

…itial run of install new package with init.ps1. It's bit complicated.

…on at all.

… code.

…t for emitting telemetry in PowerShellHost.cs

…rom init.ps1

erdembayar · 2021-01-21T16:35:28Z

@erdembayar

Can you please review the PR description.

@nkolev92

Ok. I updated PR description.

zivkan

github tells me that new commits were pushed. Publishing my comments now before I refresh to check what changed.

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/IDE/VSSolutionManager.cs

zivkan · 2021-01-21T17:22:06Z

src/NuGet.Clients/NuGet.Console/Xamls/ConsoleContainer.xaml.cs

+            NuGetPowerShellUsageCollector _nuGetPowerShellUsageCollector = ServiceLocator.GetInstance<NuGetPowerShellUsageCollector>();
+            Assumes.NotNull(_nuGetPowerShellUsageCollector);


This variable isn't being used anywhere

Thank you for review. Removed. This funky thing used for event subscription to work.

zivkan · 2021-01-21T17:23:49Z

src/NuGet.Clients/NuGet.Tools/NuGetPackage.cs

                SolutionManager.Value.NuGetProjectContext = ProjectContext.Value;
            }

+            NuGetPowerShellUsageCollector nuGetPowerShellUsageCollectorInstance = NuGetPowerShellUsageCollector.Value;


This variable isn't being used anywhere

Ok. Removed. This funky thing used for event subscription to work.

zivkan · 2021-01-21T17:30:19Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+    [Export(typeof(NuGetPowerShellUsageCollector))]
+    [PartCreationPolicy(CreationPolicy.Shared)]
+    public sealed class NuGetPowerShellUsageCollector : IDisposable


The only places this class is MEF imported, no methods or properties are being called. Therefore, it seems there's no point to make it MEF exported. However, I guess it's because you want this class to be a singleton. Given this class needs to subscrube to the events from the beginning of the NuGet package (extension) being loaded by Visual Studio, and stay alive until VS shuts down, I suggest stop exporting as MEF, and instead just new ..() in NuGetPackage.cs's InitializeAsync method, saving it as a class field, so it doesn't get garbage disposed (although if it subscribes to events, it will have references and not be eligible for GC). Just need to make sure solution events are subscribed to. If you register through NuGet's solution manager, rather than VS's "native" interfaces, then that would need to be "hooked up" correctly.

Ok. Taken your suggestion and moved to NugetPackage.cs>> InitializeAsync method. Please check if I implemented correctly.

zivkan · 2021-01-21T17:33:09Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+            NuGetPowerShellUsage.VSInstanceCloseEvent -= NuGetPowerShellUsage_VSInstanseCloseHandler;
+        }
+
+        internal class NuGetPowershellVSSolutionCloseEvent


This class class doesn't represent an event, so I don't think this is a good name. Perhaps SolutionData, SolutionMetrics, something like that.

Thank you for better name. Implemented.

zivkan · 2021-01-21T17:58:56Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+                    // Telemetry service is not ready then delay for while.
+                    if (TelemetryActivity.NuGetTelemetryService == null)
+                    {
+                        _powerShellLoadEvent = telemetryEvent;
+                    }


Initiaizing the telemetry service should be moved out of VsSolutionManager into NuGetPackage.InitializeAsync, so this situation shouldn't be possible.

Thank you for review. I implemented your suggestion.

zivkan · 2021-01-21T18:00:50Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+                {
+                    var telemetryEvent = new TelemetryEvent(NuGetPowerShellLoaded, new Dictionary<string, object>
+                        {
+                            { NuGetPowershellPrefix + LoadedFromPMC, isPMC }


Rather than a boolean, would it be better to have a vs.nuget.powershellloadreason that has values like pmc or pmui?

I would prefer leave it as it's now. Because next lines I have to make comparison to "pmc" or "pmui" which gets ugly.

I wasn't suggesting replacing the isPMC variable/parameter everywhere, just the reported telemetry:

var telemetryEvent = new TelemetryEvent(NuGetPowerShellLoaded, new Dictionary<string, object>() { { NuGetPowershellPrefix + "Trigger", isPMC ? "PMC" : "PMUI" } };

Consider the point of view from someone querying the data. They see vs.nuget.nugetpowershellprefix.loadedfrompmc == false, but if they don't NuGet very well, they might wonder where did it load from then?

This made me notice, do you really want to use NuGetPowershellPrefix for this? This has a different event name vs.nuget.nugetpowershellloaded, so duplicating nugetpowershell in the property name is redundant.

Ok.
Implemented your suggestion. Also I'm removing prefix concept. It's remnant from universal 1 telemetry which contains different types of properties from several types of activities.

zivkan · 2021-01-21T18:03:42Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+            _vsSolutionData._loadedFromPMUI = pmuiPowershellLoad;
+        }
+
+        private void IncrementUpdateVSInstanceData()


just thinking out loud, I don't know if this is a good idea or not: Would it be better to increment both the solution and instance counters in the event handler, so it's not necessary to "fixup" the instance data on solution open/close?

Ok. Removed this logic. Update InstanceData values when I update solution data same time.

zivkan · 2021-01-21T18:05:06Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+                _vsInstanceData._pmcLoadedSolutionCount++;
+            }
+
+            if (_vsSolutionData._loadedFromPMUI)
+            {
+                _vsInstanceData._pmuiLoadedSolutionCount++;


This method (IncrementUpdateVSInstanceData) is being called on both solution open and solution close. Therefore, this "solution count" will be wrong (hopefully always exactly double the real value).

Ok. Removed this logic. Update InstanceData values when I update solution data same time.

zivkan · 2021-01-21T18:10:06Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+        public const string NuGetVSSolutionClose = nameof(NuGetVSSolutionClose);
+        public const string NuGetVSInstanceClose = nameof(NuGetVSInstanceClose);


These two properties are being used as the telemetry event name. This class has a specific name, but the telemetry event name is very generic. @dominoFire do you have an opinion on what is better to query? A single telemetry event with many, unrelated properties, or use the event name to partition different "families" of data, even when they are emit at the same time (solution close, VS close)?

We can refactor this class later to make it easier to re-use a generic event name later, but it will break all data queries if we change the event name, so that's more important to get right now.

@zivkan
Your concern is valid. I can change name now. Also I added this prefix to prevent any future property conflict

If I name it not this generic then I don't need this prefix.

I consider it's better to have one event:

Triggered when it's appropriate to make ordering assumptions, but, don't rely on timing data since each client/user/machine has their relative clocks

Has all needed properties, to avoid further table joins

In spec already agreed we're going to have separate vssolution emit and vsinstance telemetry.
Also if we wait for VS close for 1 batched telemetry event then in case of PMC cause VS to fail, we'll loose all the history leading to this event.
For example: Maybe user used 1 solution with PMC powershell without problem then next another solution with PMUI powershell which cause VS to fail. We need usage history and currently we're sending aggregate data to reduce traffic.
I removed almost all nuget prefixes from properties and rename my events powershellloaded, powershellvssolutionclose, `powershellvsinstanceclose'.

erdembayar

@zivkan

I addressed PR comments. Please review again.

erdembayar · 2021-01-21T18:41:26Z

src/NuGet.Clients/NuGet.Console/Xamls/ConsoleContainer.xaml.cs

+            NuGetPowerShellUsageCollector _nuGetPowerShellUsageCollector = ServiceLocator.GetInstance<NuGetPowerShellUsageCollector>();
+            Assumes.NotNull(_nuGetPowerShellUsageCollector);


Thank you for review. Removed. This funky thing used for event subscription to work.

erdembayar · 2021-01-21T18:44:08Z

src/NuGet.Clients/NuGet.Tools/NuGetPackage.cs

                SolutionManager.Value.NuGetProjectContext = ProjectContext.Value;
            }

+            NuGetPowerShellUsageCollector nuGetPowerShellUsageCollectorInstance = NuGetPowerShellUsageCollector.Value;


Ok. Removed. This funky thing used for event subscription to work.

erdembayar · 2021-01-21T18:51:01Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+                    var telemetryEvent = new TelemetryEvent(NuGetPowerShellLoaded, new Dictionary<string, object>
+                        {
+                            { NuGetPowershellPrefix + LoadedFromPMC, isPMC }
+                        });


I thought about doing it from PMC itself, but we still need it here to check if it's first time load or not logic. So I prefer to leave it here.

erdembayar · 2021-01-21T19:55:58Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+        public const string NuGetVSSolutionClose = nameof(NuGetVSSolutionClose);
+        public const string NuGetVSInstanceClose = nameof(NuGetVSInstanceClose);


@zivkan
Your concern is valid. I can change name now. Also I added this prefix to prevent any future property conflict

If I name it not this generic then I don't need this prefix.

erdembayar · 2021-01-21T20:01:06Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+    [Export(typeof(NuGetPowerShellUsageCollector))]
+    [PartCreationPolicy(CreationPolicy.Shared)]
+    public sealed class NuGetPowerShellUsageCollector : IDisposable


Ok. Taken your suggestion and moved to NugetPackage.cs>> InitializeAsync method. Please check if I implemented correctly.

erdembayar · 2021-01-21T21:18:53Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+                _vsInstanceData._pmcLoadedSolutionCount++;
+            }
+
+            if (_vsSolutionData._loadedFromPMUI)
+            {
+                _vsInstanceData._pmuiLoadedSolutionCount++;


Ok. Removed this logic. Update InstanceData values when I update solution data same time.

erdembayar · 2021-01-21T21:19:09Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+            _vsSolutionData._loadedFromPMUI = pmuiPowershellLoad;
+        }
+
+        private void IncrementUpdateVSInstanceData()


Ok. Removed this logic. Update InstanceData values when I update solution data same time.

erdembayar · 2021-01-21T21:53:41Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+                    switch (command)
+                    {
+                        case "GET-HELP":
+                        case "FIND-PACKAGE":
+                        case "GET-PACKAGE":
+                        case "INSTALL-PACKAGE":
+                        case "UNINSTALL-PACKAGE":
+                        case "UPDATE-PACKAGE":
+                        case "SYNC-PACKAGE":
+                        case "ADD-BINDINGREDIRECT":
+                        case "GET-PROJECT":
+                        case "REGISTER-TABEXPANSION":
+                            // Nuget Command executed
+                            vsSolutionData._nuGetCommandUsed = true;
+                            break;


Ok. Created new NuGetCmdletExecutedEvent and raising it from NuGetPowerShellBaseCommand.cs.

erdembayar · 2021-01-21T21:59:37Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+                {
+                    var telemetryEvent = new TelemetryEvent(NuGetPowerShellLoaded, new Dictionary<string, object>
+                        {
+                            { NuGetPowershellPrefix + LoadedFromPMC, isPMC }


I would prefer leave it as it's now. Because next lines I have to make comparison to "pmc" or "pmui" which gets ugly.

erdembayar · 2021-01-21T22:09:07Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+                if (isPMC)
+                {
+                    {
+                        // Please note: Direct PMC and PMUI don't share same code path for installing packages with *.ps1 files
+                        // For PMC all installation done in one pass so no double counting.
+                        vsSolutionData._nuGetPMCExecuteCommandCount++;
+                    }
+                }
+                else
+                {
+                    // Please note: Direct PMC and PMUI don't share same code path for installing packages with *.ps1 files
+                    // This one is called for both init.ps1 and install.ps1 seperately.
+                    // For MSBuildNuGetProject projects install.ps1 can even further increase duplicate counting: See MSBuildNuGetProject.cs#L377 - L396
+                    // Also this concern valid for dependent packages with *.ps1 files.
+                    vsSolutionData._nuGetPMUIExecuteCommandCount++;
+                }


Ok. Addressed.

dominoFire

Some comments. Looks good

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/Powershell/NuGetPowerShellUsage.cs

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

dominoFire · 2021-01-21T23:52:45Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+        public const string NuGetVSSolutionClose = nameof(NuGetVSSolutionClose);
+        public const string NuGetVSInstanceClose = nameof(NuGetVSInstanceClose);


I consider it's better to have one event:

Triggered when it's appropriate to make ordering assumptions, but, don't rely on timing data since each client/user/machine has their relative clocks

Has all needed properties, to avoid further table joins

dominoFire · 2021-01-21T23:57:44Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+        public const string ReOpenAtStart = nameof(ReOpenAtStart);
+
+        // Const name for emitting when VS solution close or VS instance close.
+        public const string NuGetPowershellPrefix = "NuGetPowershellPrefix."; // Using prefix prevent accidental same name property collission from different type telemetry.


I would try to reduce how many times 'nuget' appears in event and property names.

public const string PackageCommands = "PackageCommands."; // Using prefix prevent accidental same name property collission from different type telemetry.

The word "prefix" in the property names doesn't add anything to the name either.

I'm not sure if PackageCommands. is the suggestion, or just a generic example, but this telemetry is not about packages, or really about commnads. It's more about NuGet's powershell host usage, so I think vs.nuget.powershellhost. would be a good prefix.

I think now no need anymore. I'm getting rid of prefix then all together. It's remnant from universal vssolution/vsinstance emitter idea.

From this comment thread, Fernando suggested keeping the generic "vs.nuget.solutionclose" and "vs.nuget.instanceclose" event names, so we can reuse it for other data in the future where we want to report at solution/instance close.

erdembayar

@dominoFire
Thank you for your review. Implemented all your suggestion except batching all telemetry only 1 event emit.
In spec already agreed we're going to have separate vssolution emit and vsinstance telemetry.
Also if we wait for VS close for 1 batched telemetry event then in case of PMC cause VS to fail, we'll loose all the history leading to this event.
For example: Maybe user used 1 solution with PMC powershell without problem then next another solution with PMUI powershell which cause VS to fail. We need usage history and currently we're sending aggregate data to reduce traffic.
I removed almost all nuget prefixes from properties and rename my events powershellloaded, powershellvssolutionclose, `powershellvsinstanceclose'.
Please check again.

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/Powershell/NuGetPowerShellUsage.cs

zivkan · 2021-01-22T03:11:54Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+        public const string ReOpenAtStart = nameof(ReOpenAtStart);
+
+        // Const name for emitting when VS solution close or VS instance close.
+        public const string NuGetPowershellPrefix = "NuGetPowershellPrefix."; // Using prefix prevent accidental same name property collission from different type telemetry.


The word "prefix" in the property names doesn't add anything to the name either.

I'm not sure if PackageCommands. is the suggestion, or just a generic example, but this telemetry is not about packages, or really about commnads. It's more about NuGet's powershell host usage, so I think vs.nuget.powershellhost. would be a good prefix.

zivkan · 2021-01-22T03:13:11Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+        // _vsSolutionData hold telemetry data for current VS solution session.
+        private SolutionData _vsSolutionData;
+        // _vsInstanceData hold telemetry for current VS instance session.
+        private readonly InstanceData _vsInstanceData;


I'm not sure what other people think, but I think the variable names are clear enough, so the comments don't add value.

Sure, removing.

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

zivkan · 2021-01-22T03:19:34Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+    public sealed class NuGetPowerShellUsageCollector : IDisposable
+    {
+        // PMC, PMUI powershell telemetry consts
+        public const string NuGetPMCExecuteCommandCount = nameof(NuGetPMCExecuteCommandCount);


As mentioned in another comment, initializations should only have the first letter capitalized, the rest lower case, like Sql, Xml, Json. I see many, many instances of all upper PMC and PMUI in this file. Please check the whole PR.

Not sure why only the first letter capitalized?
I checked previous examples how it's implemented. All of them using Pascal case like mine.

NuGet.Client/src/NuGet.Core/NuGet.PackageManagement/Projects/ProjectBuildProperties.cs

Lines 12 to 17 in a8521b9

public const string MSBuildProjectExtensionsPath = nameof(MSBuildProjectExtensionsPath);

public const string PackageTargetFallback = nameof(PackageTargetFallback);

public const string AssetTargetFallback = nameof(AssetTargetFallback);

public const string PackageVersion = nameof(PackageVersion);

public const string RestoreProjectStyle = nameof(RestoreProjectStyle);

public const string RuntimeIdentifier = nameof(RuntimeIdentifier);

NuGet.Client/src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

Lines 38 to 40 in a8521b9

private static readonly string AggregateSourceName = Resources.AggregateSourceName;

private static readonly TimeSpan ExecuteInitScriptsRetryDelay = TimeSpan.FromMilliseconds(400);

private static readonly int MaxTasks = 16;

I feel current form makes easy to read, in my previous company they enforced it so it became habit. If only the first letter capitalized is better practice then I can change to that one.

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions#capitalization-rules-for-identifiers

The PascalCasing convention, used for all identifiers except parameter names, capitalizes the first character of each word (including acronyms over two letters in length), as shown in the following examples:

PropertyDescriptor HtmlTag

A special case is made for two-letter acronyms in which both letters are capitalized, as shown in the following identifier:

IOStream

This is why we have APIs like:

SqlClient, not SQLClient

XmlDocument, not XMLDocument

JsonSerializer, not JSONSerializer.

So, NuGetPMCExecuteCommandCount should be NuGetPmcExecuteCommandCount. There are many examples of this naming convention not being followed in this PR for both Pmc and Pmui. When writing human text, we continue to capitalize the full "word", so I'd still talk about PMC, but code using it as part of a variable name should use Pmc.

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/Powershell/NuGetPowerShellUsage.cs

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

erdembayar

@zivkan
Thank you for your review. I addressed all your comments. There 2-3 things I commented my point of view.
Please check.

erdembayar · 2021-01-22T04:01:46Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+    public sealed class NuGetPowerShellUsageCollector : IDisposable
+    {
+        // PMC, PMUI powershell telemetry consts
+        public const string NuGetPMCExecuteCommandCount = nameof(NuGetPMCExecuteCommandCount);


Not sure why only the first letter capitalized?
I checked previous examples how it's implemented. All of them using Pascal case like mine.

NuGet.Client/src/NuGet.Core/NuGet.PackageManagement/Projects/ProjectBuildProperties.cs

Lines 12 to 17 in a8521b9

public const string MSBuildProjectExtensionsPath = nameof(MSBuildProjectExtensionsPath);

public const string PackageTargetFallback = nameof(PackageTargetFallback);

public const string AssetTargetFallback = nameof(AssetTargetFallback);

public const string PackageVersion = nameof(PackageVersion);

public const string RestoreProjectStyle = nameof(RestoreProjectStyle);

public const string RuntimeIdentifier = nameof(RuntimeIdentifier);

NuGet.Client/src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

Lines 38 to 40 in a8521b9

private static readonly string AggregateSourceName = Resources.AggregateSourceName;

private static readonly TimeSpan ExecuteInitScriptsRetryDelay = TimeSpan.FromMilliseconds(400);

private static readonly int MaxTasks = 16;

I feel current form makes easy to read, in my previous company they enforced it so it became habit. If only the first letter capitalized is better practice then I can change to that one.

erdembayar · 2021-01-22T04:03:41Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+        public const string ReOpenAtStart = nameof(ReOpenAtStart);
+
+        // Const name for emitting when VS solution close or VS instance close.
+        public const string NuGetPowershellPrefix = "NuGetPowershellPrefix."; // Using prefix prevent accidental same name property collission from different type telemetry.


I think now no need anymore. I'm getting rid of prefix then all together. It's remnant from universal vssolution/vsinstance emitter idea.

erdembayar · 2021-01-22T04:04:45Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+        public const string NuGetVSSolutionClose = nameof(NuGetVSSolutionClose);
+        public const string NuGetVSInstanceClose = nameof(NuGetVSInstanceClose);


In spec already agreed we're going to have separate vssolution emit and vsinstance telemetry.
Also if we wait for VS close for 1 batched telemetry event then in case of PMC cause VS to fail, we'll loose all the history leading to this event.
For example: Maybe user used 1 solution with PMC powershell without problem then next another solution with PMUI powershell which cause VS to fail. We need usage history and currently we're sending aggregate data to reduce traffic.
I removed almost all nuget prefixes from properties and rename my events powershellloaded, powershellvssolutionclose, `powershellvsinstanceclose'.

erdembayar · 2021-01-22T04:05:00Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+        // _vsSolutionData hold telemetry data for current VS solution session.
+        private SolutionData _vsSolutionData;
+        // _vsInstanceData hold telemetry for current VS instance session.
+        private readonly InstanceData _vsInstanceData;


Sure, removing.

erdembayar · 2021-01-22T04:08:34Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+                {
+                    var telemetryEvent = new TelemetryEvent(NuGetPowerShellLoaded, new Dictionary<string, object>
+                        {
+                            { NuGetPowershellPrefix + LoadedFromPMC, isPMC }


Ok.
Implemented your suggestion. Also I'm removing prefix concept. It's remnant from universal 1 telemetry which contains different types of properties from several types of activities.

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/Powershell/NuGetPowerShellUsage.cs

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

zivkan · 2021-01-22T17:48:03Z

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

+    public sealed class NuGetPowerShellUsageCollector : IDisposable
+    {
+        // PMC, PMUI powershell telemetry consts
+        public const string NuGetPMCExecuteCommandCount = nameof(NuGetPMCExecuteCommandCount);


https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions#capitalization-rules-for-identifiers

The PascalCasing convention, used for all identifiers except parameter names, capitalizes the first character of each word (including acronyms over two letters in length), as shown in the following examples:

PropertyDescriptor HtmlTag

A special case is made for two-letter acronyms in which both letters are capitalized, as shown in the following identifier:

IOStream

This is why we have APIs like:

SqlClient, not SQLClient

XmlDocument, not XMLDocument

JsonSerializer, not JSONSerializer.

So, NuGetPMCExecuteCommandCount should be NuGetPmcExecuteCommandCount. There are many examples of this naming convention not being followed in this PR for both Pmc and Pmui. When writing human text, we continue to capitalize the full "word", so I'd still talk about PMC, but code using it as part of a variable name should use Pmc.

zivkan

Once we sort out the "generic solution/instance close with appropriate property name prefix" vs "specific telemetry events without any property name prefix" decision/implementation, I think this will be good enough to go.

dominoFire

Some styling comments.

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

…event called vs/nuget/solutionclose and vs instance close event called vs/nuget/instanceclose.

erdembayar

@zivkan
Thank you for your review. I addressed all latest comments. Please review again.

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

nkolev92 · 2021-01-23T00:51:03Z

src/NuGet.Clients/NuGet.Tools/NuGetPackage.cs

        protected override async Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress)
        {
+            _nuGetPowerShellUsageCollector = new NuGetPowerShellUsageCollector();
+            NuGet.Common.TelemetryActivity.NuGetTelemetryService = new NuGetVSTelemetryService();


Rule 26 https://github.com/NuGet/NuGet.Client/blob/dev/docs/coding-guidelines.md

@nkolev92
Reason I did it was if I import Nuget.Common then just 8-9 lines down on AsyncLazy<IVsMonitorSelection> needed exact same thing it needs fully qualified name. So instead of touching existing code for same change I tried to contain my changes.

You can always do a using like this one: https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.VisualStudio.Common/OutputConsoleLogger.cs#L14

Addressed in new PR#3863.

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetPowerShellUsageCollector.cs

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

nkolev92 · 2021-01-23T01:24:42Z

@erdembayar

Please consider waiting on reviewers that have been engaged throughout the whole PR process.

I left some feedback that you should consider addressing in a follow up PR.

erdembayar · 2021-01-23T03:50:10Z

@erdembayar

Please consider waiting on reviewers that have been engaged throughout the whole PR process.

I left some feedback that you should consider addressing in a follow up PR.

@nkolev92
Sorry, I saw your PR comments right after I merged. I created this follow up PR, please check. #3863

erdembayar force-pushed the dev-eryondon-PMC-telemetry-event branch from 79a27ab to 099c7f0 Compare November 10, 2020 02:18

erdembayar closed this Nov 12, 2020

erdembayar reopened this Nov 12, 2020

erdembayar marked this pull request as ready for review November 12, 2020 18:59

erdembayar requested a review from a team as a code owner November 12, 2020 18:59

erdembayar force-pushed the dev-eryondon-PMC-telemetry-event branch 4 times, most recently from a90ff26 to c1feb67 Compare November 12, 2020 23:59

erdembayar added 21 commits November 12, 2020 21:18

Event to track how many times commands are executed on PMC.

1834814

Batch count of commands executed on PMC.

594fad9

Fix format.

a8cb55d

Add telemetry for how many times PMC was openened.

ef02445

Add telemetry for if PMC opened by default without user gesture. Add telemetry for how many times powershell commands executed.

Add code comment for automatically loaded PMC.

2b2e4a6

There are 2 seperate instances of PowerShellHost.cs can be created fo…

b0476a6

…r PM UI and PM PMC. So make change to accommodiate this concern.

Combine 2 events make less number of event (just 1) to emit.

6a2cb41

Fix typos.

a3945de

Add actual origin which caused init.ps1 to run into telemetry.

287c0c6

Make names of telemetry event a constants.

a9d8ffd

Get donnie's advise for naming.

2061923

Remove init.ps1 telemetry because current scenario doesn't capture in…

57e8963

…itial run of install new package with init.ps1. It's bit complicated.

Address Andy's code review comment.

e7e032b

Remove unused variable.

22300d4

Send single event for both PMUI and PMC powershell events.

9127d0a

Make powershell telemetry works for with and without solution load.

be473af

No need to emit separate nugetvssolutionclose if there were no soluti…

8190d66

…on at all.

Refactor code so it's not much mix too much telemetry code into other…

a265545

… code.

Add telemetry detecting Nuget command and load of init.ps1 for packages.

a99b667

Move telemetry logic into seperate class VsPowerShellHostTelemetryEmi…

bbe8502

…t for emitting telemetry in PowerShellHost.cs

Fix formatting.

3a139c4

erdembayar added 3 commits January 20, 2021 18:24

Correct window count logic.

d057453

Improve formatting and fix PMC, PMUI loaded not set from after load f…

f25af0e

…rom init.ps1

Correct solution count.

eaa9b9f

Address PR comment Nkolche.

9ab746d

zivkan reviewed Jan 21, 2021

View reviewed changes

Address remaining review comments from Andy.

73fc122

erdembayar commented Jan 21, 2021

View reviewed changes

erdembayar added 2 commits January 21, 2021 15:18

Clean up fix and fix typos.

ec4ae8d

Fix if PMC opened without solution then that window load is not counted.

9179b9c

dominoFire reviewed Jan 21, 2021

View reviewed changes

Address PR comments by Fernando.

99b450d

erdembayar commented Jan 22, 2021

View reviewed changes

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs Outdated Show resolved Hide resolved

src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/Powershell/NuGetPowerShellUsage.cs Outdated Show resolved Hide resolved

zivkan reviewed Jan 22, 2021

View reviewed changes

Address latest Andy's comments (removing prefix etc)

ccf74a0

erdembayar commented Jan 22, 2021

View reviewed changes

zivkan reviewed Jan 22, 2021

View reviewed changes

dominoFire approved these changes Jan 22, 2021

View reviewed changes

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs Outdated Show resolved Hide resolved

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs Outdated Show resolved Hide resolved

erdembayar added 2 commits January 22, 2021 13:16

Address latest PR comment from Andy and Fernando. Now solution close …

aa0ba5b

…event called vs/nuget/solutionclose and vs instance close event called vs/nuget/instanceclose.

Fix naming conventions.

a9607e7

erdembayar commented Jan 22, 2021

View reviewed changes

zivkan approved these changes Jan 22, 2021

View reviewed changes

erdembayar merged commit 7c710c0 into dev Jan 23, 2021

erdembayar deleted the dev-eryondon-PMC-telemetry-event branch January 23, 2021 01:18

nkolev92 reviewed Jan 23, 2021

View reviewed changes

This was referenced Jan 23, 2021

Implementation - Add new telemetries to track events related to PMC and Powershell usage follow up. NuGet/Home#10478

Closed

Address missing PR comments for Add new telemetries to track events related to PMC and Powershell usage #3863

Merged

		NuGetPowerShellUsageCollector _nuGetPowerShellUsageCollector = ServiceLocator.GetInstance<NuGetPowerShellUsageCollector>();
		Assumes.NotNull(_nuGetPowerShellUsageCollector);

		public const string NuGetVSSolutionClose = nameof(NuGetVSSolutionClose);
		public const string NuGetVSInstanceClose = nameof(NuGetVSInstanceClose);

	public const string MSBuildProjectExtensionsPath = nameof(MSBuildProjectExtensionsPath);
	public const string PackageTargetFallback = nameof(PackageTargetFallback);
	public const string AssetTargetFallback = nameof(AssetTargetFallback);
	public const string PackageVersion = nameof(PackageVersion);
	public const string RestoreProjectStyle = nameof(RestoreProjectStyle);
	public const string RuntimeIdentifier = nameof(RuntimeIdentifier);

	private static readonly string AggregateSourceName = Resources.AggregateSourceName;
	private static readonly TimeSpan ExecuteInitScriptsRetryDelay = TimeSpan.FromMilliseconds(400);
	private static readonly int MaxTasks = 16;

Understand PMC and Powershell usages #3755

Understand PMC and Powershell usages #3755

Uh oh!

Conversation

erdembayar commented Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Fix

Testing/Validation

Uh oh!

erdembayar commented Jan 21, 2021

Uh oh!

zivkan left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

erdembayar 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!

Choose a reason for hiding this comment

erdembayar commented Nov 10, 2020 •

edited

Loading