From f6b01fa552920a84f1707de7473a580eb9e34ec3 Mon Sep 17 00:00:00 2001 From: Robert Schumacher Date: Fri, 13 Aug 2021 22:59:37 -0700 Subject: [PATCH 1/5] Only consult applocal userconfig if metrics are enabled --- src/vcpkg.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/vcpkg.cpp b/src/vcpkg.cpp index 8ffceda10a..17434235c7 100644 --- a/src/vcpkg.cpp +++ b/src/vcpkg.cpp @@ -206,8 +206,6 @@ int main(const int argc, const char* const* const argv) register_console_ctrl_handler(); - load_config(fs); - #if (defined(__aarch64__) || defined(__arm__) || defined(__s390x__) || \ ((defined(__ppc64__) || defined(__PPC64__) || defined(__ppc64le__) || defined(__PPC64LE__)) && \ defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)) || \ @@ -243,6 +241,11 @@ int main(const int argc, const char* const* const argv) metrics->set_disabled(true); } + if (metrics->metrics_enabled()) + { + load_config(fs); + } + if (const auto p = args.print_metrics.get()) { metrics->set_print_metrics(*p); From bdb2b788557d38443274a2a8a954469485d608cb Mon Sep 17 00:00:00 2001 From: Robert Schumacher Date: Wed, 18 Aug 2021 00:40:54 +0000 Subject: [PATCH 2/5] Fix nested locking --- src/vcpkg.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/vcpkg.cpp b/src/vcpkg.cpp index 17434235c7..cf4eeeeb78 100644 --- a/src/vcpkg.cpp +++ b/src/vcpkg.cpp @@ -225,6 +225,7 @@ int main(const int argc, const char* const* const argv) VcpkgCmdArguments::imbue_or_apply_process_recursion(args); args.check_feature_flag_consistency(); + bool metrics_enabled; { LockGuardPtr metrics(g_metrics); if (const auto p = args.disable_metrics.get()) @@ -241,10 +242,7 @@ int main(const int argc, const char* const* const argv) metrics->set_disabled(true); } - if (metrics->metrics_enabled()) - { - load_config(fs); - } + metrics_enabled = metrics->metrics_enabled(); if (const auto p = args.print_metrics.get()) { @@ -262,6 +260,11 @@ int main(const int argc, const char* const* const argv) } } // unlock g_metrics + if (metrics_enabled) + { + load_config(fs); + } + args.debug_print_feature_flags(); args.track_feature_flag_metrics(); From 9b32037d0df646e76898ad205a9d2a9b8dbe56f3 Mon Sep 17 00:00:00 2001 From: Robert Schumacher Date: Fri, 5 Nov 2021 01:06:29 +0000 Subject: [PATCH 3/5] Address CR feedback --- include/vcpkg/metrics.h | 6 +-- src/vcpkg.cpp | 83 +++++++++------------------------------ src/vcpkg/metrics.cpp | 87 +++++++++++++++++++++++------------------ 3 files changed, 67 insertions(+), 109 deletions(-) diff --git a/include/vcpkg/metrics.h b/include/vcpkg/metrics.h index 8516342fd6..2d43502f34 100644 --- a/include/vcpkg/metrics.h +++ b/include/vcpkg/metrics.h @@ -16,9 +16,7 @@ namespace vcpkg void set_send_metrics(bool should_send_metrics); void set_print_metrics(bool should_print_metrics); - void set_disabled(bool disabled); - void set_user_information(const std::string& user_id, const std::string& first_use_time); - static void init_user_information(std::string& user_id, std::string& first_use_time); + void enable(); void track_metric(const std::string& name, double value); void track_buildtime(const std::string& name, double value); @@ -32,6 +30,4 @@ namespace vcpkg }; extern LockGuarded g_metrics; - - std::string get_MAC_user(); } diff --git a/src/vcpkg.cpp b/src/vcpkg.cpp index cf4eeeeb78..c12dcf821f 100644 --- a/src/vcpkg.cpp +++ b/src/vcpkg.cpp @@ -16,7 +16,6 @@ #include #include #include -#include #include #include #include @@ -97,50 +96,6 @@ static void inner(vcpkg::Filesystem& fs, const VcpkgCmdArguments& args) return invalid_command(args.command); } -static void load_config(vcpkg::Filesystem& fs) -{ - auto config = UserConfig::try_read_data(fs); - - bool write_config = false; - - // config file not found, could not be read, or invalid - if (config.user_id.empty() || config.user_time.empty()) - { - ::vcpkg::Metrics::init_user_information(config.user_id, config.user_time); - write_config = true; - } - -#if defined(_WIN32) - if (config.user_mac.empty()) - { - config.user_mac = get_MAC_user(); - write_config = true; - } -#endif - - { - LockGuardPtr locked_metrics(g_metrics); - locked_metrics->set_user_information(config.user_id, config.user_time); -#if defined(_WIN32) - locked_metrics->track_property("user_mac", config.user_mac); -#endif - } - - if (config.last_completed_survey.empty()) - { - const auto now = CTime::parse(config.user_time).value_or_exit(VCPKG_LINE_INFO); - const CTime offset = now.add_hours(-SURVEY_INITIAL_OFFSET_IN_HOURS); - config.last_completed_survey = offset.to_string(); - } - - LockGuardPtr(GlobalState::g_surveydate)->assign(config.last_completed_survey); - - if (write_config) - { - config.try_write_data(fs); - } -} - #if defined(_WIN32) // note: this prevents a false positive for -Wmissing-prototypes on clang-cl int wmain(int, const wchar_t* const* const); @@ -225,25 +180,28 @@ int main(const int argc, const char* const* const argv) VcpkgCmdArguments::imbue_or_apply_process_recursion(args); args.check_feature_flag_consistency(); - bool metrics_enabled; + bool to_enable_metrics = true; + if (const auto p = args.disable_metrics.get()) { - LockGuardPtr metrics(g_metrics); - if (const auto p = args.disable_metrics.get()) - { - metrics->set_disabled(*p); - } + to_enable_metrics = false; + } - auto disable_metrics_tag_file_path = get_exe_path_of_current_process(); - disable_metrics_tag_file_path.replace_filename("vcpkg.disable-metrics"); + auto disable_metrics_tag_file_path = get_exe_path_of_current_process(); + disable_metrics_tag_file_path.replace_filename("vcpkg.disable-metrics"); - std::error_code ec; - if (fs.exists(disable_metrics_tag_file_path, ec) || ec) + std::error_code ec; + if (fs.exists(disable_metrics_tag_file_path, ec) || ec) + { + to_enable_metrics = false; + } + + { + LockGuardPtr metrics(g_metrics); + if (to_enable_metrics) { - metrics->set_disabled(true); + metrics->enable(); } - metrics_enabled = metrics->metrics_enabled(); - if (const auto p = args.print_metrics.get()) { metrics->set_print_metrics(*p); @@ -253,16 +211,11 @@ int main(const int argc, const char* const* const argv) { metrics->set_send_metrics(*p); } - - if (args.send_metrics.value_or(false) && !metrics->metrics_enabled()) - { - print2(Color::warning, "Warning: passed --sendmetrics, but metrics are disabled.\n"); - } } // unlock g_metrics - if (metrics_enabled) + if (args.send_metrics.value_or(false) && !to_enable_metrics) { - load_config(fs); + print2(Color::warning, "Warning: passed --sendmetrics, but metrics are disabled.\n"); } args.debug_print_feature_flags(); diff --git a/src/vcpkg/metrics.cpp b/src/vcpkg/metrics.cpp index e14d0081ae..22c676fc8c 100644 --- a/src/vcpkg/metrics.cpp +++ b/src/vcpkg/metrics.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #if defined(_WIN32) #pragma comment(lib, "version") @@ -247,11 +248,11 @@ namespace vcpkg #endif ; static bool g_should_print_metrics = false; - static bool g_metrics_disabled = false; + static bool g_metrics_disabled = true; - std::string get_MAC_user() - { #if defined(_WIN32) + static std::string get_MAC_user() + { if (!LockGuardPtr(g_metrics)->metrics_enabled()) { return "{}"; @@ -276,66 +277,74 @@ namespace vcpkg } return "0"; -#else - return "{}"; -#endif } +#endif - void Metrics::set_user_information(const std::string& user_id, const std::string& first_use_time) + static void load_config(Metrics& m, vcpkg::Filesystem& fs) { - g_metricmessage.user_id = user_id; - g_metricmessage.user_timestamp = first_use_time; - } + (void)m; + auto config = UserConfig::try_read_data(fs); - void Metrics::init_user_information(std::string& user_id, std::string& first_use_time) - { - user_id = generate_random_UUID(); - first_use_time = get_current_date_time_string(); + bool write_config = false; + + // config file not found, could not be read, or invalid + if (config.user_id.empty() || config.user_time.empty()) + { + config.user_id = generate_random_UUID(); + config.user_time = get_current_date_time_string(); + write_config = true; + } + +#if defined(_WIN32) + if (config.user_mac.empty()) + { + config.user_mac = get_MAC_user(); + write_config = true; + } +#endif + + g_metricmessage.user_id = config.user_id; + g_metricmessage.user_timestamp = config.user_time; + +#if defined(_WIN32) + m.track_property("user_mac", config.user_mac); +#endif + + if (write_config) + { + config.try_write_data(fs); + } } void Metrics::set_send_metrics(bool should_send_metrics) { g_should_send_metrics = should_send_metrics; } void Metrics::set_print_metrics(bool should_print_metrics) { g_should_print_metrics = should_print_metrics; } - void Metrics::set_disabled(bool disabled) { g_metrics_disabled = disabled; } - - bool Metrics::metrics_enabled() { return !g_metrics_disabled; } - - void Metrics::track_metric(const std::string& name, double value) + void Metrics::enable() { - if (!metrics_enabled()) + if (g_metrics_disabled) { - return; + // Ensure load_config() is called exactly once + load_config(*this, get_real_filesystem()); } - g_metricmessage.track_metric(name, value); + g_metrics_disabled = false; } + bool Metrics::metrics_enabled() { return !g_metrics_disabled; } + + void Metrics::track_metric(const std::string& name, double value) { g_metricmessage.track_metric(name, value); } + void Metrics::track_buildtime(const std::string& name, double value) { - if (!metrics_enabled()) - { - return; - } g_metricmessage.track_buildtime(name, value); } void Metrics::track_property(const std::string& name, const std::string& value) { - if (!metrics_enabled()) - { - return; - } g_metricmessage.track_property(name, value); } - void Metrics::track_feature(const std::string& name, bool value) - { - if (!metrics_enabled()) - { - return; - } - g_metricmessage.track_feature(name, value); - } + void Metrics::track_feature(const std::string& name, bool value) { g_metricmessage.track_feature(name, value); } void Metrics::upload(const std::string& payload) { @@ -432,7 +441,7 @@ namespace vcpkg if (request) WinHttpCloseHandle(request); if (connect) WinHttpCloseHandle(connect); if (session) WinHttpCloseHandle(session); -#else // ^^^ _WIN32 // !_WIN32 vvv +#else // ^^^ _WIN32 // !_WIN32 vvv (void)payload; #endif // ^^^ !_WIN32 } From 3c4f1925d5a3448c815213d98112d4e61872763d Mon Sep 17 00:00:00 2001 From: Robert Schumacher Date: Sat, 13 Nov 2021 20:39:42 +0000 Subject: [PATCH 4/5] Respect --no-disable-metrics --- src/vcpkg.cpp | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/vcpkg.cpp b/src/vcpkg.cpp index 8b5a76293a..6ee60b357b 100644 --- a/src/vcpkg.cpp +++ b/src/vcpkg.cpp @@ -58,12 +58,6 @@ CMD=)"); DECLARE_AND_REGISTER_MESSAGE(VcpkgHasCrashedArgument, (msg::value), "{LOCKED}", "{value}|"); } -// 24 hours/day * 30 days/month * 6 months -static constexpr int SURVEY_INTERVAL_IN_HOURS = 24 * 30 * 6; - -// Initial survey appears after 10 days. Therefore, subtract 24 hours/day * 10 days -static constexpr int SURVEY_INITIAL_OFFSET_IN_HOURS = SURVEY_INTERVAL_IN_HOURS - 24 * 10; - static void invalid_command(const std::string& cmd) { msg::println(Color::error, msgVcpkgInvalidCommand, msg::value = cmd); @@ -243,11 +237,6 @@ int main(const int argc, const char* const* const argv) args.check_feature_flag_consistency(); bool to_enable_metrics = true; - if (const auto p = args.disable_metrics.get()) - { - to_enable_metrics = false; - } - auto disable_metrics_tag_file_path = get_exe_path_of_current_process(); disable_metrics_tag_file_path.replace_filename("vcpkg.disable-metrics"); @@ -257,6 +246,11 @@ int main(const int argc, const char* const* const argv) to_enable_metrics = false; } + if (auto p = args.disable_metrics.get()) + { + to_enable_metrics = !*p; + } + { LockGuardPtr metrics(g_metrics); if (to_enable_metrics) From aa40a41b3b091d1d727c2eb7969d9978253a47b3 Mon Sep 17 00:00:00 2001 From: Robert Schumacher Date: Sun, 14 Nov 2021 19:59:54 +0000 Subject: [PATCH 5/5] Reorganize locks to support failure during metrics initialization --- include/vcpkg/metrics.h | 4 +++- src/vcpkg.cpp | 10 +++++----- src/vcpkg/metrics.cpp | 44 ++++++++++++++++++++++------------------- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/include/vcpkg/metrics.h b/include/vcpkg/metrics.h index 2d43502f34..db9c0ed5da 100644 --- a/include/vcpkg/metrics.h +++ b/include/vcpkg/metrics.h @@ -16,7 +16,9 @@ namespace vcpkg void set_send_metrics(bool should_send_metrics); void set_print_metrics(bool should_print_metrics); - void enable(); + + // This function is static and must be called outside the g_metrics lock. + static void enable(); void track_metric(const std::string& name, double value); void track_buildtime(const std::string& name, double value); diff --git a/src/vcpkg.cpp b/src/vcpkg.cpp index 6ee60b357b..38c2be308e 100644 --- a/src/vcpkg.cpp +++ b/src/vcpkg.cpp @@ -251,13 +251,13 @@ int main(const int argc, const char* const* const argv) to_enable_metrics = !*p; } + if (to_enable_metrics) { - LockGuardPtr metrics(g_metrics); - if (to_enable_metrics) - { - metrics->enable(); - } + Metrics::enable(); + } + { + LockGuardPtr metrics(g_metrics); if (const auto p = args.print_metrics.get()) { metrics->set_print_metrics(*p); diff --git a/src/vcpkg/metrics.cpp b/src/vcpkg/metrics.cpp index 22c676fc8c..385541dc7f 100644 --- a/src/vcpkg/metrics.cpp +++ b/src/vcpkg/metrics.cpp @@ -248,7 +248,7 @@ namespace vcpkg #endif ; static bool g_should_print_metrics = false; - static bool g_metrics_disabled = true; + static std::atomic g_metrics_disabled = true; #if defined(_WIN32) static std::string get_MAC_user() @@ -280,9 +280,22 @@ namespace vcpkg } #endif - static void load_config(Metrics& m, vcpkg::Filesystem& fs) + void Metrics::set_send_metrics(bool should_send_metrics) { g_should_send_metrics = should_send_metrics; } + + void Metrics::set_print_metrics(bool should_print_metrics) { g_should_print_metrics = should_print_metrics; } + + static bool g_initializing_metrics = false; + + void Metrics::enable() { - (void)m; + { + LockGuardPtr metrics(g_metrics); + if (g_initializing_metrics) return; + g_initializing_metrics = true; + } + + // Execute this body exactly once + auto& fs = get_real_filesystem(); auto config = UserConfig::try_read_data(fs); bool write_config = false; @@ -303,31 +316,22 @@ namespace vcpkg } #endif - g_metricmessage.user_id = config.user_id; - g_metricmessage.user_timestamp = config.user_time; - -#if defined(_WIN32) - m.track_property("user_mac", config.user_mac); -#endif - if (write_config) { config.try_write_data(fs); } - } - void Metrics::set_send_metrics(bool should_send_metrics) { g_should_send_metrics = should_send_metrics; } + { + LockGuardPtr metrics(g_metrics); + g_metricmessage.user_id = config.user_id; + g_metricmessage.user_timestamp = config.user_time; - void Metrics::set_print_metrics(bool should_print_metrics) { g_should_print_metrics = should_print_metrics; } +#if defined(_WIN32) + metrics->track_property("user_mac", config.user_mac); +#endif - void Metrics::enable() - { - if (g_metrics_disabled) - { - // Ensure load_config() is called exactly once - load_config(*this, get_real_filesystem()); + g_metrics_disabled = false; } - g_metrics_disabled = false; } bool Metrics::metrics_enabled() { return !g_metrics_disabled; }