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

Prevent circular dependency in global telemetry logger creation #1674

Merged
merged 3 commits into from
Nov 10, 2021
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
32 changes: 28 additions & 4 deletions src/AppInstallerCommonCore/AppInstallerTelemetry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -553,7 +576,8 @@ namespace AppInstaller::Logging
}
else
{
static GlobalTelemetryTraceLogger processGlobalTelemetry;
static TelemetryTraceLogger processGlobalTelemetry;
processGlobalTelemetry.TryInitialize();
return processGlobalTelemetry;
}
}
Expand Down
18 changes: 11 additions & 7 deletions src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -131,13 +139,16 @@ 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;
std::wstring AnonymizeString(std::wstring_view input) const noexcept;

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"{}";
Expand All @@ -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();

Expand Down
3 changes: 1 addition & 2 deletions src/AppInstallerCommonCore/Public/winget/ThreadGlobals.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 2 additions & 4 deletions src/AppInstallerCommonCore/Public/winget/UserSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,6 @@ namespace AppInstaller::Settings
~UserSettings() = default;
};

inline UserSettings const& User()
{
return UserSettings::Instance();
}
const UserSettings* TryGetUser();
UserSettings const& User();
}
2 changes: 1 addition & 1 deletion src/AppInstallerCommonCore/ThreadGlobals.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
4 changes: 2 additions & 2 deletions src/AppInstallerCommonCore/TraceLogger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
}
Expand All @@ -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"));
}
Expand Down
34 changes: 32 additions & 2 deletions src/AppInstallerCommonCore/UserSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
10 changes: 8 additions & 2 deletions src/WinGetUtil/Exports.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this may solve the problem when the code stays on one thread, it may make other problems that we are not aware of. I would rather we fix it completely by creating a set of APIs that give more explicit control over the thread globals so that callers can actually control what is happening.

// 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);
Expand Down