Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt to prevent crash in TelemetryTraceLogger::InitializeInternal() #3527

Merged
merged 3 commits into from
Aug 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/AppInstallerCommonCore/AppInstallerTelemetry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -809,7 +812,6 @@ namespace AppInstaller::Logging
void TelemetryTraceLogger::InitializeInternal(const AppInstaller::Settings::UserSettings& userSettings)
{
m_isSettingEnabled = !userSettings.Get<Settings::Setting::TelemetryDisable>();
m_userProfile = Runtime::GetPathTo(Runtime::PathName::UserProfile).wstring();
m_isInitialized = true;
}

Expand All @@ -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 }; }

Expand All @@ -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;
Expand Down
18 changes: 14 additions & 4 deletions src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -255,15 +259,24 @@ 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.
// Should primarily be used on failure messages or paths if needed.
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<bool> m_isSettingEnabled{ false };

// We may decide to disable telemetry at runtime, for example, for command line completion.
CopyConstructibleAtomic<bool> m_isRuntimeEnabled{ true };

// We wait for initialization of the other flags before sending any events.
CopyConstructibleAtomic<bool> m_isInitialized{ false };

CopyConstructibleAtomic<uint32_t> m_executionStage{ 0 };
Expand All @@ -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;

Expand Down