diff --git a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp index 38457f62e7..2f149a6353 100644 --- a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp +++ b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp @@ -31,6 +31,7 @@ namespace AppInstaller::Logging static const uint32_t s_RootExecutionId = 0; static std::atomic_uint32_t s_subExecutionId{ s_RootExecutionId }; + // Data that is needed by AnonymizeString constexpr std::wstring_view s_UserProfileReplacement = L"%USERPROFILE%"sv; // TODO: Temporary code to keep existing telemetry behavior @@ -121,6 +122,8 @@ namespace AppInstaller::Logging { if (!m_isInitialized) { + // Only initialize if we already have the user settings, so that we can respect the telemetry setting. + // We may not yet have the user settings if we are trying to report an error while reading them. auto userSettings = Settings::TryGetUser(); if (userSettings) { @@ -809,7 +812,6 @@ namespace AppInstaller::Logging void TelemetryTraceLogger::InitializeInternal(const AppInstaller::Settings::UserSettings& userSettings) { m_isSettingEnabled = !userSettings.Get(); - m_userProfile = Runtime::GetPathTo(Runtime::PathName::UserProfile).wstring(); m_isInitialized = true; } @@ -820,7 +822,11 @@ namespace AppInstaller::Logging std::wstring TelemetryTraceLogger::AnonymizeString(std::wstring_view input) const noexcept try { - return Utility::ReplaceWhileCopying(input, m_userProfile, s_UserProfileReplacement); + // GetPathTo() may need to read the settings, so this function should only be called after settings are initialized. + // To ensure that, this function is only called when emitting an event, and we disable the telemetry until settings are ready. + static const std::wstring s_UserProfile = Runtime::GetPathTo(Runtime::PathName::UserProfile).wstring(); + + return Utility::ReplaceWhileCopying(input, s_UserProfile, s_UserProfileReplacement); } catch (...) { return std::wstring{ input }; } @@ -843,6 +849,9 @@ namespace AppInstaller::Logging } else { + // For the global telemetry object, we may not have yet read the settings file. + // In that case, we will not be able to initialize it, so we need to try it + // each time we get the object. static TelemetryTraceLogger processGlobalTelemetry(/* useSummary */ false); processGlobalTelemetry.TryInitialize(); return processGlobalTelemetry; diff --git a/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h b/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h index 25176bb800..c794b0ba03 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h @@ -154,6 +154,10 @@ namespace AppInstaller::Logging void Initialize(); // Try to capture if UserSettings is enabled and set user profile path, returns whether the action is successfully completed. + // There is a possible circular dependency with the user settings. When initializing the telemetry, we need to read the settings + // to make sure it's not disabled, but a failure when reading the settings would trigger a telemetry event. We work around that + // by avoiding initialization (and thus disabling telemetry) until we have successfully read the settings. Subsequent calls to + // TryInitialize() would finish the initialization. bool TryInitialize(); // Store the passed in name of the Caller for COM calls @@ -255,6 +259,7 @@ namespace AppInstaller::Logging protected: bool IsTelemetryEnabled() const noexcept; + // Initializes flags that determine whether telemetry is enabled. void InitializeInternal(const AppInstaller::Settings::UserSettings& userSettings); // Used to anonymize a string to the best of our ability. @@ -262,8 +267,16 @@ namespace AppInstaller::Logging std::wstring AnonymizeString(const wchar_t* input) const noexcept; std::wstring AnonymizeString(std::wstring_view input) const noexcept; - bool m_isSettingEnabled = true; + // Flags used to determine whether to send telemetry. All of them are set during initialization and + // are CopyConstructibleAtomic to minimize the impact of multiple simultaneous initialization attempts. + // m_isSettingEnabled starts as false so we can don't send telemetry until we have read the + // settings and confirmed that it is enabled. + CopyConstructibleAtomic m_isSettingEnabled{ false }; + + // We may decide to disable telemetry at runtime, for example, for command line completion. CopyConstructibleAtomic m_isRuntimeEnabled{ true }; + + // We wait for initialization of the other flags before sending any events. CopyConstructibleAtomic m_isInitialized{ false }; CopyConstructibleAtomic m_executionStage{ 0 }; @@ -273,9 +286,6 @@ namespace AppInstaller::Logging std::wstring m_telemetryCorrelationJsonW = L"{}"; std::string m_caller; - // Data that is needed by AnonymizeString - std::wstring m_userProfile; - bool m_useSummary = true; mutable TelemetrySummary m_summary;