winget-cli

commit 297c9f1108691fd9fb0e0b3c71a7f920ac015d29
parent 7cc506872c52ef7104b37ee9ca13ac8dd50a1f9c
Author: JohnMcPMS <johnmcp@microsoft.com>
Date:   Wed, 21 Dec 2022 16:04:20 -0800

Don't treat the COM subcommands as distinct for telemetry (#2792)

This change treats the `OrchestratorQueueItem` as the command for telemetry purposes.  This aligns with the `winget.exe` method of counting success/failure.  It also moves to use the same command names as `winget.exe`, making it easier to join/slice the data.
Diffstat:
M
src/AppInstallerCLICore/Command.cpp
 | 
7
++++++-
M
src/AppInstallerCLICore/Command.h
 | 
2
++
M
src/AppInstallerCLICore/ContextOrchestrator.cpp
 | 
39
+++++++++++++++++++++++++++++----------
M
src/AppInstallerCLICore/ContextOrchestrator.h
 | 
5
+++--

4 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp
@@ -907,7 +907,7 @@ namespace AppInstaller::CLI
         return arguments;
     }
 
-    int Execute(Execution::Context& context, std::unique_ptr<Command>& command)
+    void ExecuteWithoutLoggingSuccess(Execution::Context& context, Command* command)
     {
         try
         {
@@ -923,6 +923,11 @@ namespace AppInstaller::CLI
         {
             context.SetTerminationHR(Workflow::HandleException(context, std::current_exception()));
         }
+    }
+
+    int Execute(Execution::Context& context, std::unique_ptr<Command>& command)
+    {
+        ExecuteWithoutLoggingSuccess(context, command.get());
 
         if (SUCCEEDED(context.GetTerminationHR()))
         {
diff --git a/src/AppInstallerCLICore/Command.h b/src/AppInstallerCLICore/Command.h
@@ -140,5 +140,7 @@ namespace AppInstaller::CLI
         return result;
     }
 
+    void ExecuteWithoutLoggingSuccess(Execution::Context& context, Command* command);
+
     int Execute(Execution::Context& context, std::unique_ptr<Command>& command);
 }
diff --git a/src/AppInstallerCLICore/ContextOrchestrator.cpp b/src/AppInstallerCLICore/ContextOrchestrator.cpp
@@ -91,6 +91,9 @@ namespace AppInstaller::CLI::Execution
         if (item->IsOnFirstCommand())
         {
             THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INSTALL_ALREADY_RUNNING), FindById(item->GetId()));
+
+            // Log the beginning of the item
+            item->GetContext().GetThreadGlobals().GetTelemetryLogger().LogCommand(item->GetItemCommandName());
         }
 
         std::string commandQueueName{ GetCommandQueueName(item->GetNextCommand().Name()) };
@@ -246,34 +249,37 @@ namespace AppInstaller::CLI::Execution
             }
 
             // Get the item's command and execute it.
-            HRESULT terminationHR = S_OK;
+            HRESULT exceptionHR = S_OK;
             try
             {
                 std::unique_ptr<Command> command = item->PopNextCommand();
 
                 std::unique_ptr<AppInstaller::ThreadLocalStorage::PreviousThreadGlobals> setThreadGlobalsToPreviousState = item->GetContext().SetForCurrentThread();
 
-                item->GetContext().GetThreadGlobals().GetTelemetryLogger().LogCommand(command->FullName());
                 command->ValidateArguments(item->GetContext().Args);
 
                 item->GetContext().EnableCtrlHandler();
 
-                terminationHR = ::AppInstaller::CLI::Execute(item->GetContext(), command);
+                ::AppInstaller::CLI::ExecuteWithoutLoggingSuccess(item->GetContext(), command.get());
             }
-            WINGET_CATCH_STORE(terminationHR, APPINSTALLER_CLI_ERROR_COMMAND_FAILED);
+            WINGET_CATCH_STORE(exceptionHR, APPINSTALLER_CLI_ERROR_COMMAND_FAILED);
 
-            if (FAILED(terminationHR))
+            if (FAILED(exceptionHR))
             {
-                // ::Execute sometimes catches exceptions and returns hresults based on those exceptions without the context
-                // being updated with that hresult. This sets the termination hr directly so that the context always 
+                // Set the termination hr directly from any exception that escaped so that the context always 
                 // has the result of the operation no matter how it failed.
-                item->GetContext().SetTerminationHR(terminationHR);
+                item->GetContext().SetTerminationHR(exceptionHR);
             }
 
             item->GetContext().EnableCtrlHandler(false);
 
-            if (FAILED(terminationHR) || item->IsComplete())
+            if (FAILED(item->GetContext().GetTerminationHR()) || item->IsComplete())
             {
+                if (SUCCEEDED(item->GetContext().GetTerminationHR()))
+                {
+                    item->GetContext().GetThreadGlobals().GetTelemetryLogger().LogCommandSuccess(item->GetItemCommandName());
+                }
+
                 RemoveItemInState(*item, OrchestratorQueueItemState::Running, true);
             }
             else
@@ -334,6 +340,19 @@ namespace AppInstaller::CLI::Execution
                 (GetSourceId() == comparedId.GetSourceId()));
     }
 
+    std::string_view OrchestratorQueueItem::GetItemCommandName() const
+    {
+        // The goal is that these should match the winget.exe commands for easy correlation.
+        switch (m_operationType)
+        {
+        case PackageOperationType::Search: return "root:search"sv;
+        case PackageOperationType::Install: return "root:install"sv;
+        case PackageOperationType::Upgrade: return "root:upgrade"sv;
+        case PackageOperationType::Uninstall: return "root:uninstall"sv;
+        default: return "unknown";
+        }
+    }
+
     std::unique_ptr<OrchestratorQueueItem> OrchestratorQueueItemFactory::CreateItemForInstall(std::wstring packageId, std::wstring sourceId, std::unique_ptr<COMContext> context, bool isUpgrade)
     {
         std::unique_ptr<OrchestratorQueueItem> item = std::make_unique<OrchestratorQueueItem>(OrchestratorQueueItemId(std::move(packageId), std::move(sourceId)), std::move(context), isUpgrade ? PackageOperationType::Upgrade : PackageOperationType::Install);
@@ -351,7 +370,7 @@ namespace AppInstaller::CLI::Execution
 
     std::unique_ptr<OrchestratorQueueItem> OrchestratorQueueItemFactory::CreateItemForSearch(std::wstring packageId, std::wstring sourceId, std::unique_ptr<COMContext> context)
     {
-        std::unique_ptr<OrchestratorQueueItem> item = std::make_unique<OrchestratorQueueItem>(OrchestratorQueueItemId(std::move(packageId), std::move(sourceId)), std::move(context), PackageOperationType::None);
+        std::unique_ptr<OrchestratorQueueItem> item = std::make_unique<OrchestratorQueueItem>(OrchestratorQueueItemId(std::move(packageId), std::move(sourceId)), std::move(context), PackageOperationType::Search);
         return item;
     }
 }
diff --git a/src/AppInstallerCLICore/ContextOrchestrator.h b/src/AppInstallerCLICore/ContextOrchestrator.h
@@ -42,7 +42,7 @@ namespace AppInstaller::CLI::Execution
 
     enum class PackageOperationType
     {
-        None,
+        Search,
         Install,
         Upgrade,
         Uninstall,
@@ -77,6 +77,7 @@ namespace AppInstaller::CLI::Execution
         bool IsComplete() const { return m_commands.empty(); }
         bool IsApplicableForInstallingSource() const { return m_operationType == PackageOperationType::Install || m_operationType == PackageOperationType::Upgrade; }
         PackageOperationType GetPackageOperationType() const { return m_operationType; }
+        std::string_view GetItemCommandName() const;
 
     private:
         OrchestratorQueueItemState m_state = OrchestratorQueueItemState::NotQueued;
@@ -86,7 +87,7 @@ namespace AppInstaller::CLI::Execution
         std::deque<std::unique_ptr<Command>> m_commands;
         bool m_isOnFirstCommand = true;
         OrchestratorQueue* m_currentQueue = nullptr;
-        PackageOperationType m_operationType = PackageOperationType::None;
+        PackageOperationType m_operationType;
     };
 
     struct OrchestratorQueueItemFactory