commit b4693b191463df328d21021a17cc2b09b665ef33
parent a14de79013927fab44f529fd2299a99414d3b059
Author: yao-msft <50888816+yao-msft@users.noreply.github.com>
Date: Tue, 9 Nov 2021 17:27:55 -0800
Prevent circular dependency in global telemetry logger creation (#1674)
Diffstat:
8 files changed, 85 insertions(+), 24 deletions(-)
diff --git a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp
@@ -80,8 +80,24 @@ namespace AppInstaller::Logging
void TelemetryTraceLogger::Initialize()
{
- m_isSettingEnabled = !Settings::User().Get<Settings::Setting::TelemetryDisable>();
- m_userProfile = Runtime::GetPathTo(Runtime::PathName::UserProfile).wstring();
+ if (!m_isInitialized)
+ {
+ InitializeInternal(Settings::User());
+ }
+ }
+
+ bool TelemetryTraceLogger::TryInitialize()
+ {
+ if (!m_isInitialized)
+ {
+ auto userSettings = Settings::TryGetUser();
+ if (userSettings)
+ {
+ InitializeInternal(*userSettings);
+ }
+ }
+
+ return m_isInitialized;
}
void TelemetryTraceLogger::SetTelemetryCorrelationJson(const std::wstring_view jsonStr_view) noexcept
@@ -520,7 +536,14 @@ namespace AppInstaller::Logging
bool TelemetryTraceLogger::IsTelemetryEnabled() const noexcept
{
- return g_IsTelemetryProviderEnabled && m_isSettingEnabled && m_isRuntimeEnabled;
+ return g_IsTelemetryProviderEnabled && m_isInitialized && m_isSettingEnabled && m_isRuntimeEnabled;
+ }
+
+ 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;
}
std::wstring TelemetryTraceLogger::AnonymizeString(const wchar_t* input) const noexcept
@@ -553,7 +576,8 @@ namespace AppInstaller::Logging
}
else
{
- static GlobalTelemetryTraceLogger processGlobalTelemetry;
+ static TelemetryTraceLogger processGlobalTelemetry;
+ processGlobalTelemetry.TryInitialize();
return processGlobalTelemetry;
}
}
diff --git a/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h b/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h
@@ -8,6 +8,11 @@
#include <vector>
#include <cguid.h>
+namespace AppInstaller::Settings
+{
+ struct UserSettings;
+}
+
namespace AppInstaller::Logging
{
// This type contains the registration lifetime of the telemetry trace logging provider.
@@ -36,6 +41,9 @@ namespace AppInstaller::Logging
// Capture if UserSettings is enabled and set user profile path
void Initialize();
+ // Try to capture if UserSettings is enabled and set user profile path, returns whether the action is successfully completed.
+ bool TryInitialize();
+
// Store the passed in name of the Caller for COM calls
void SetCaller(const std::string& caller);
@@ -131,6 +139,8 @@ namespace AppInstaller::Logging
protected:
bool IsTelemetryEnabled() const noexcept;
+ 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;
@@ -138,6 +148,7 @@ namespace AppInstaller::Logging
bool m_isSettingEnabled = true;
std::atomic_bool m_isRuntimeEnabled{ true };
+ std::atomic_bool m_isInitialized{ false };
GUID m_activityId = GUID_NULL;
std::wstring m_telemetryCorrelationJsonW = L"{}";
@@ -147,13 +158,6 @@ namespace AppInstaller::Logging
std::wstring m_userProfile;
};
- struct GlobalTelemetryTraceLogger : TelemetryTraceLogger
- {
- GlobalTelemetryTraceLogger() { Initialize(); }
-
- ~GlobalTelemetryTraceLogger() = default;
- };
-
// Helper to make the call sites look clean.
TelemetryTraceLogger& Telemetry();
diff --git a/src/AppInstallerCommonCore/Public/winget/ThreadGlobals.h b/src/AppInstallerCommonCore/Public/winget/ThreadGlobals.h
@@ -26,13 +26,12 @@ namespace AppInstaller::ThreadLocalStorage
static ThreadGlobals* GetForCurrentThread();
private:
-
+
void Initialize();
std::unique_ptr<AppInstaller::Logging::DiagnosticLogger> m_pDiagnosticLogger;
std::unique_ptr<AppInstaller::Logging::TelemetryTraceLogger> m_pTelemetryLogger;
std::once_flag loggerInitOnceFlag;
-
};
struct PreviousThreadGlobals
diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h
@@ -188,8 +188,6 @@ namespace AppInstaller::Settings
~UserSettings() = default;
};
- inline UserSettings const& User()
- {
- return UserSettings::Instance();
- }
+ const UserSettings* TryGetUser();
+ UserSettings const& User();
}
diff --git a/src/AppInstallerCommonCore/ThreadGlobals.cpp b/src/AppInstallerCommonCore/ThreadGlobals.cpp
@@ -35,7 +35,7 @@ namespace AppInstaller::ThreadLocalStorage
{
m_pDiagnosticLogger = std::make_unique<DiagnosticLogger>();
m_pTelemetryLogger = std::make_unique<TelemetryTraceLogger>();
-
+
// The above make_unique for TelemetryTraceLogger will either create an object or will throw which is caught below.
m_pTelemetryLogger->Initialize();
});
diff --git a/src/AppInstallerCommonCore/TraceLogger.cpp b/src/AppInstallerCommonCore/TraceLogger.cpp
@@ -15,7 +15,7 @@ namespace AppInstaller::Logging
TraceLoggingWriteActivity(g_hTraceProvider,
"Diagnostics",
- nullptr, // TODO: ActivityId of the Global and COMContext telemetry to be logged in future
+ Telemetry().GetActivityId(),
nullptr,
TraceLoggingString(strstr.str().c_str(), "LogMessage"));
}
@@ -28,7 +28,7 @@ namespace AppInstaller::Logging
{
TraceLoggingWriteActivity(g_hTraceProvider,
"Diagnostics",
- nullptr, // TODO: ActivityId of the Global and COMContext telemetry to be logged in future
+ Telemetry().GetActivityId(),
nullptr,
TraceLoggingCountedUtf8String(message.data(), static_cast<ULONG>(message.size()), "LogMessage"));
}
diff --git a/src/AppInstallerCommonCore/UserSettings.cpp b/src/AppInstallerCommonCore/UserSettings.cpp
@@ -305,20 +305,50 @@ namespace AppInstaller::Settings
}
#endif
+ static std::atomic_bool s_userSettingsInitialized{ false };
+ static std::atomic_bool s_userSettingsInInitialization{ false };
+
UserSettings const& UserSettings::Instance()
{
- static UserSettings userSettings;
-
#ifndef AICLI_DISABLE_TEST_HOOKS
if (s_UserSettings_Override)
{
return *s_UserSettings_Override;
}
#endif
+ if (!s_userSettingsInitialized)
+ {
+ s_userSettingsInInitialization = true;
+ }
+
+ static UserSettings userSettings;
+ s_userSettingsInitialized = true;
+ s_userSettingsInInitialization = false;
return userSettings;
}
+ const UserSettings* TryGetUser()
+ {
+ if (s_userSettingsInitialized)
+ {
+ return &UserSettings::Instance();
+ }
+
+ // Try to initialize UserSettings, return nullptr if it's already in initialization.
+ if (s_userSettingsInInitialization)
+ {
+ return nullptr;
+ }
+
+ return &UserSettings::Instance();
+ }
+
+ UserSettings const& User()
+ {
+ return UserSettings::Instance();
+ }
+
UserSettings::UserSettings() : m_type(UserSettingsType::Default)
{
Json::Value settingsRoot = Json::Value::nullSingleton();
diff --git a/src/WinGetUtil/Exports.cpp b/src/WinGetUtil/Exports.cpp
@@ -10,6 +10,7 @@
#include <AppInstallerTelemetry.h>
#include <Microsoft/SQLiteIndex.h>
#include <winget/ManifestYamlParser.h>
+#include <winget/ThreadGlobals.h>
using namespace AppInstaller::Utility;
using namespace AppInstaller::Manifest;
@@ -21,8 +22,13 @@ extern "C"
{
THROW_HR_IF(E_INVALIDARG, !logPath);
- static std::once_flag s_initLogging;
- std::call_once(s_initLogging, []() {
+ thread_local AppInstaller::ThreadLocalStorage::ThreadGlobals threadGlobals;
+ thread_local std::once_flag initLogging;
+
+ std::call_once(initLogging, []() {
+ std::unique_ptr<AppInstaller::ThreadLocalStorage::PreviousThreadGlobals> previous = threadGlobals.SetForCurrentThread();
+ // Intentionally release to leave the local ThreadGlobals.
+ previous.release();
// Enable all logs for now.
AppInstaller::Logging::Log().EnableChannel(AppInstaller::Logging::Channel::All);
AppInstaller::Logging::Log().SetLevel(AppInstaller::Logging::Level::Verbose);
