[telemetry] Refactor track_property() (#685)

* initialize timestamp in metrics message

* refactor track_property()

* Update src/vcpkg/vcpkgpaths.cpp

* Remove `track_option()`

* Renamed SetMetric to DefineMetric

* Never trust VS's `Ctrl+R,Ctrl+R`

* Add missing metric name

* format

* Partially address Robert's comments

* Change `track_property()` call sites

* Review comments and tests

* Update src/vcpkg-test/metrics.cpp

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

* simplify tests

* Remove `track_property()` overrides

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

include/vcpkg/metrics.h

@@ -8,6 +8,68 @@
 
 namespace vcpkg
 {
+    template<typename T>
+    struct MetricEntry
+    {
+        T metric;
+        StringLiteral name;
+    };
+
+    enum class DefineMetric
+    {
+        AssetSource,
+        BinaryCachingAws,
+        BinaryCachingAzBlob,
+        BinaryCachingCos,
+        BinaryCachingDefault,
+        BinaryCachingFiles,
+        BinaryCachingGcs,
+        BinaryCachingHttp,
+        BinaryCachingNuget,
+        BinaryCachingSource,
+        ErrorVersioningDisabled,
+        ErrorVersioningNoBaseline,
+        GitHubRepository,
+        ManifestBaseline,
+        ManifestOverrides,
+        ManifestVersionConstraint,
+        RegistriesErrorCouldNotFindBaseline,
+        RegistriesErrorNoVersionsAtCommit,
+        VcpkgBinarySources,
+        VcpkgDefaultBinaryCache,
+        VcpkgNugetRepository,
+        VersioningErrorBaseline,
+        VersioningErrorVersion,
+        X_VcpkgRegistriesCache,
+        X_WriteNugetPackagesConfig,
+        COUNT // always keep COUNT last
+    };
+
+    enum class StringMetric
+    {
+        BuildError,
+        CommandArgs,
+        CommandContext,
+        CommandName,
+        Error,
+        InstallPlan_1,
+        ListFile,
+        RegistriesDefaultRegistryKind,
+        RegistriesKindsUsed,
+        Title,
+        UserMac,
+        VcpkgVersion,
+        Warning,
+        COUNT // always keep COUNT last
+    };
+
+    enum class BoolMetric
+    {
+        InstallManifestMode,
+        OptionOverlayPorts,
+        COUNT // always keep COUNT last
+    };
+
     struct Metrics
     {
         Metrics() = default;
@@ -22,18 +84,23 @@ namespace vcpkg
 
         void track_metric(const std::string& name, double value);
         void track_buildtime(const std::string& name, double value);
-        void track_property(const std::string& name, const std::string& value);
-        void track_property(const std::string& name, bool value);
+
+        void track_define_property(DefineMetric metric);
+        void track_string_property(StringMetric metric, StringView value);
+        void track_bool_property(BoolMetric metric, bool value);
+
         void track_feature(const std::string& feature, bool value);
-        void track_option(const std::string& option, bool value);
 
         bool metrics_enabled();
 
         void upload(const std::string& payload);
         void flush(Filesystem& fs);
-    };
 
-    Optional<StringView> find_first_nonzero_mac(StringView sv);
+        // exposed for testing
+        static View<MetricEntry<DefineMetric>> get_define_metrics();
+        static View<MetricEntry<StringMetric>> get_string_metrics();
+        static View<MetricEntry<BoolMetric>> get_bool_metrics();
+    };
 
     extern LockGuarded<Metrics> g_metrics;
 }

src/vcpkg-test/metrics.cpp

@@ -0,0 +1,48 @@
+#include <catch2/catch.hpp>
+
+#include <vcpkg/metrics.h>
+
+#include <set>
+
+using namespace vcpkg;
+
+template<typename T>
+void validate_enum_values_and_names(View<MetricEntry<T>> entries, const size_t expected_size)
+{
+    REQUIRE(expected_size == entries.size());
+
+    size_t enum_value = 0;
+    std::set<StringView> used_names;
+    for (auto&& m : entries)
+    {
+        // fails when a metric is not in the right order in the entries array
+        // - check that there are no duplicate or skipped metric entries
+        // - check that the order in Metrics::get_<T>_metrics() and in the <T>Metric enum is the same
+        REQUIRE(static_cast<size_t>(m.metric) == enum_value);
+        ++enum_value;
+
+        // fails when there's a repeated metric name
+        REQUIRE(!m.name.empty());
+        auto it_names = used_names.find(m.name);
+        REQUIRE(it_names == used_names.end());
+        used_names.insert(m.name);
+    }
+}
+
+TEST_CASE ("Check metric enum types", "[metrics]")
+{
+    SECTION ("define metrics")
+    {
+        validate_enum_values_and_names(Metrics::get_define_metrics(), static_cast<size_t>(DefineMetric::COUNT));
+    }
+
+    SECTION ("string metrics")
+    {
+        validate_enum_values_and_names(Metrics::get_string_metrics(), static_cast<size_t>(StringMetric::COUNT));
+    }
+
+    SECTION ("bool metrics")
+    {
+        validate_enum_values_and_names(Metrics::get_bool_metrics(), static_cast<size_t>(BoolMetric::COUNT));
+    }
+}

src/vcpkg.cpp

@@ -44,7 +44,8 @@ static void invalid_command(const std::string& cmd)
 static void inner(vcpkg::Filesystem& fs, const VcpkgCmdArguments& args)
 {
     // track version on each invocation
-    LockGuardPtr<Metrics>(g_metrics)->track_property("vcpkg_version", Commands::Version::version.to_string());
+    LockGuardPtr<Metrics>(g_metrics)->track_string_property(StringMetric::VcpkgVersion,
+                                                            Commands::Version::version.to_string());
 
     if (args.command.empty())
     {
@@ -67,11 +68,11 @@ static void inner(vcpkg::Filesystem& fs, const VcpkgCmdArguments& args)
         }
     };
 
-    LockGuardPtr<Metrics>(g_metrics)->track_option("overlay_ports", !args.overlay_ports.empty());
+    LockGuardPtr<Metrics>(g_metrics)->track_bool_property(BoolMetric::OptionOverlayPorts, !args.overlay_ports.empty());
 
     if (const auto command_function = find_command(Commands::get_available_basic_commands()))
     {
-        LockGuardPtr<Metrics>(g_metrics)->track_property("command_name", command_function->name);
+        LockGuardPtr<Metrics>(g_metrics)->track_string_property(StringMetric::CommandName, command_function->name);
         return command_function->function->perform_and_exit(args, fs);
     }
 
@@ -82,7 +83,7 @@ static void inner(vcpkg::Filesystem& fs, const VcpkgCmdArguments& args)
 
     if (const auto command_function = find_command(Commands::get_available_paths_commands()))
     {
-        LockGuardPtr<Metrics>(g_metrics)->track_property("command_name", command_function->name);
+        LockGuardPtr<Metrics>(g_metrics)->track_string_property(StringMetric::CommandName, command_function->name);
         return command_function->function->perform_and_exit(args, paths);
     }
 
@@ -93,7 +94,7 @@ static void inner(vcpkg::Filesystem& fs, const VcpkgCmdArguments& args)
 
     if (const auto command_function = find_command(Commands::get_available_triplet_commands()))
     {
-        LockGuardPtr<Metrics>(g_metrics)->track_property("command_name", command_function->name);
+        LockGuardPtr<Metrics>(g_metrics)->track_string_property(StringMetric::CommandName, command_function->name);
         return command_function->function->perform_and_exit(args, paths, default_triplet, host_triplet);
     }
 
@@ -316,7 +317,7 @@ int main(const int argc, const char* const* const argv)
         exc_msg = "unknown error(...)";
     }
 
-    LockGuardPtr<Metrics>(g_metrics)->track_property("error", exc_msg);
+    LockGuardPtr<Metrics>(g_metrics)->track_string_property(StringMetric::Error, exc_msg);
 
     fflush(stdout);
     msg::println(msgVcpkgHasCrashed);

src/vcpkg/binarycaching.cpp

@@ -1579,7 +1579,7 @@ namespace
             auto maybe_cachepath = get_environment_variable("VCPKG_DEFAULT_BINARY_CACHE");
             if (auto p_str = maybe_cachepath.get())
             {
-                LockGuardPtr<Metrics>(g_metrics)->track_property("VCPKG_DEFAULT_BINARY_CACHE", "defined");
+                LockGuardPtr<Metrics>(g_metrics)->track_define_property(DefineMetric::VcpkgDefaultBinaryCache);
                 Path path = *p_str;
                 path.make_preferred();
                 if (!get_real_filesystem().is_directory(path))
@@ -1997,9 +1997,24 @@ namespace
                 return add_error(msg::format(msgUnknownBinaryProviderType), segments[0].first);
             }
 
+            static const std::map<StringLiteral, DefineMetric> metric_names{
+                {"aws", DefineMetric::BinaryCachingAws},
+                {"azblob", DefineMetric::BinaryCachingAzBlob},
+                {"cos", DefineMetric::BinaryCachingCos},
+                {"default", DefineMetric::BinaryCachingDefault},
+                {"files", DefineMetric::BinaryCachingFiles},
+                {"gcs", DefineMetric::BinaryCachingGcs},
+                {"http", DefineMetric::BinaryCachingHttp},
+                {"nuget", DefineMetric::BinaryCachingNuget},
+            };
+            auto metrics = LockGuardPtr<Metrics>(g_metrics);
             for (const auto& cache_provider : state->binary_cache_providers)
             {
-                LockGuardPtr<Metrics>(g_metrics)->track_property("binarycaching_" + cache_provider, "defined");
+                auto it = metric_names.find(cache_provider);
+                if (it != metric_names.end())
+                {
+                    metrics->track_define_property(it->second);
+                }
             }
         }
     };
@@ -2132,7 +2147,7 @@ ExpectedS<DownloadManagerConfig> vcpkg::parse_download_configuration(const Optio
 {
     if (!arg || arg.get()->empty()) return DownloadManagerConfig{};
 
-    LockGuardPtr<Metrics>(g_metrics)->track_property("asset-source", "defined");
+    LockGuardPtr<Metrics>(g_metrics)->track_define_property(DefineMetric::AssetSource);
 
     AssetSourcesState s;
     AssetSourcesParser parser(*arg.get(), Strings::concat("$", VcpkgCmdArguments::ASSET_SOURCES_ENV), &s);
@@ -2187,12 +2202,12 @@ ExpectedS<BinaryConfigParserState> vcpkg::create_binary_providers_from_configs_p
         LockGuardPtr<Metrics> metrics(g_metrics);
         if (!env_string.empty())
         {
-            metrics->track_property("VCPKG_BINARY_SOURCES", "defined");
+            metrics->track_define_property(DefineMetric::VcpkgBinarySources);
         }
 
         if (args.size() != 0)
         {
-            metrics->track_property("binarycaching-source", "defined");
+            metrics->track_define_property(DefineMetric::BinaryCachingSource);
         }
     }
 
@@ -2331,7 +2346,7 @@ details::NuGetRepoInfo details::get_nuget_repo_info_from_env()
     auto vcpkg_nuget_repository = get_environment_variable("VCPKG_NUGET_REPOSITORY");
     if (auto p = vcpkg_nuget_repository.get())
     {
-        LockGuardPtr<Metrics>(g_metrics)->track_property("VCPKG_NUGET_REPOSITORY", "defined");
+        LockGuardPtr<Metrics>(g_metrics)->track_define_property(DefineMetric::VcpkgNugetRepository);
         return {std::move(*p)};
     }
 
@@ -2347,7 +2362,7 @@ details::NuGetRepoInfo details::get_nuget_repo_info_from_env()
         return {};
     }
 
-    LockGuardPtr<Metrics>(g_metrics)->track_property("GITHUB_REPOSITORY", "defined");
+    LockGuardPtr<Metrics>(g_metrics)->track_define_property(DefineMetric::GitHubRepository);
     return {Strings::concat(gh_server, '/', gh_repo, ".git"),
             get_environment_variable("GITHUB_REF").value_or(""),
             get_environment_variable("GITHUB_SHA").value_or("")};

src/vcpkg/build.cpp

@@ -947,8 +947,8 @@ namespace vcpkg
                                      buildtimeus);
             if (!succeeded(return_code))
             {
-                metrics->track_property("error", "build failed");
-                metrics->track_property("build_error", spec_string);
+                metrics->track_string_property(StringMetric::Error, "build failed");
+                metrics->track_string_property(StringMetric::BuildError, spec_string);
                 const auto logs = buildpath / Strings::concat("error-logs-", action.spec.triplet(), ".txt");
                 std::vector<std::string> error_logs;
                 if (fs.exists(logs, VCPKG_LINE_INFO))

src/vcpkg/commands.add.cpp

@@ -46,8 +46,8 @@ namespace vcpkg::Commands
             auto artifact_hash = Hash::get_string_hash(artifact_name, Hash::Algorithm::Sha256);
             {
                 auto metrics = LockGuardPtr<Metrics>(g_metrics);
-                metrics->track_property("command_context", "artifact");
-                metrics->track_property("command_args", artifact_hash);
+                metrics->track_string_property(StringMetric::CommandContext, "artifact");
+                metrics->track_string_property(StringMetric::CommandArgs, artifact_hash);
             } // unlock g_metrics
 
             std::string ce_args[] = {"add", artifact_name};
@@ -122,8 +122,8 @@ namespace vcpkg::Commands
                                                    }));
             {
                 auto metrics = LockGuardPtr<Metrics>(g_metrics);
-                metrics->track_property("command_context", "port");
-                metrics->track_property("command_args", command_args_hash);
+                metrics->track_string_property(StringMetric::CommandContext, "port");
+                metrics->track_string_property(StringMetric::CommandArgs, command_args_hash);
             } // unlock metrics
 
             Checks::exit_success(VCPKG_LINE_INFO);

src/vcpkg/commands.find.cpp

@@ -237,10 +237,10 @@ namespace vcpkg::Commands
             auto args_hash = Hash::get_string_hash(filter.value_or_exit(VCPKG_LINE_INFO), Hash::Algorithm::Sha256);
             {
                 auto metrics = LockGuardPtr<Metrics>(g_metrics);
-                metrics->track_property("command_context", "artifact");
+                metrics->track_string_property(StringMetric::CommandContext, "artifact");
                 if (auto p_filter_hash = filter_hash.get())
                 {
-                    metrics->track_property("command_args", *p_filter_hash);
+                    metrics->track_string_property(StringMetric::CommandArgs, *p_filter_hash);
                 }
             } // unlock metrics
 
@@ -252,10 +252,10 @@ namespace vcpkg::Commands
             Optional<std::string> filter_hash = filter.map(Hash::get_string_sha256);
             {
                 auto metrics = LockGuardPtr<Metrics>(g_metrics);
-                metrics->track_property("command_context", "port");
+                metrics->track_string_property(StringMetric::CommandContext, "port");
                 if (auto p_filter_hash = filter_hash.get())
                 {
-                    metrics->track_property("command_args", *p_filter_hash);
+                    metrics->track_string_property(StringMetric::CommandArgs, *p_filter_hash);
                 }
             } // unlock metrics
 

src/vcpkg/commands.integrate.cpp

@@ -472,8 +472,8 @@ namespace vcpkg::Commands::Integrate
                                    .append_raw("\n" + script_path.generic_u8string()));
             {
                 auto locked_metrics = LockGuardPtr<Metrics>(g_metrics);
-                locked_metrics->track_property("error", "powershell script failed");
-                locked_metrics->track_property("title", TITLE.to_string());
+                locked_metrics->track_string_property(StringMetric::Error, "powershell script failed");
+                locked_metrics->track_string_property(StringMetric::Title, TITLE.to_string());
             }
         }
 

src/vcpkg/commands.setinstalled.cpp

@@ -164,7 +164,7 @@ namespace vcpkg::Commands::SetInstalled
         auto it_pkgsconfig = options.settings.find(OPTION_WRITE_PACKAGES_CONFIG);
         if (it_pkgsconfig != options.settings.end())
         {
-            LockGuardPtr<Metrics>(g_metrics)->track_property("x-write-nuget-packages-config", "defined");
+            LockGuardPtr<Metrics>(g_metrics)->track_define_property(DefineMetric::X_WriteNugetPackagesConfig);
             pkgsconfig = it_pkgsconfig->second;
         }
 

src/vcpkg/input.cpp

@@ -35,7 +35,8 @@ namespace vcpkg
         if (!paths.is_valid_triplet(t))
         {
             print2(Color::error, "Error: invalid triplet: ", t, '\n');
-            LockGuardPtr<Metrics>(g_metrics)->track_property("error", "invalid triplet: " + t.to_string());
+            LockGuardPtr<Metrics>(g_metrics)->track_string_property(StringMetric::Error,
+                                                                    "invalid triplet: " + t.to_string());
             Help::help_topic_valid_triplet(paths);
             Checks::exit_fail(VCPKG_LINE_INFO);
         }