Skip to content

[metrics] Add new telemetry points for versioning #21

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.

Sign up for GitHub

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

Merged
vicroms merged 2 commits into from
Mar 16, 2021
Merged

[metrics] Add new telemetry points for versioning #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f135625
Add new versioning telemetry points
vicroms Mar 2, 2021
abef65d
Remove telemetry on generic install plan error
vicroms Mar 2, 2021
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@

/.idea
/cmake-build-*
/.vs
/CMakeSettings.json
1 change: 1 addition & 0 deletions src/vcpkg/dependencies.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include <vcpkg/cmakevars.h>
#include <vcpkg/dependencies.h>
#include <vcpkg/metrics.h>
#include <vcpkg/packagespec.h>
#include <vcpkg/paragraphs.h>
#include <vcpkg/portfileprovider.h>
Expand Down
10 changes: 10 additions & 0 deletions src/vcpkg/install.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,17 +874,26 @@ namespace vcpkg::Install
}
}

if (std::any_of(dependencies.begin(), dependencies.end(), [](const Dependency& dep) {
return dep.constraint.type != Versions::Constraint::Type::None;
}))
{
Metrics::g_metrics.lock()->track_property("manifest_version_constraint", "defined");
}

if (!manifest_scf.core_paragraph->overrides.empty())
{
Metrics::g_metrics.lock()->track_property("manifest_overrides", "defined");
}

if (auto p_baseline = manifest_scf.core_paragraph->builtin_baseline.get())
{
Metrics::g_metrics.lock()->track_property("manifest_baseline", "defined");
if (p_baseline->size() != 40 || !std::all_of(p_baseline->begin(), p_baseline->end(), [](char ch) {
return (ch >= 'a' || ch <= 'f') || Parse::ParserBase::is_ascii_digit(ch);
}))
{
Metrics::g_metrics.lock()->track_property("versioning-error-baseline", "defined");
Checks::exit_maybe_upgrade(VCPKG_LINE_INFO,
"Error: the top-level builtin-baseline (%s) was not a valid commit sha: "
"expected 40 lowercase hexadecimal characters.\n%s\n",
Expand All @@ -894,6 +903,7 @@ namespace vcpkg::Install

paths.get_configuration().registry_set.experimental_set_builtin_registry_baseline(*p_baseline);
}

auto verprovider = PortFileProvider::make_versioned_portfile_provider(paths);
auto baseprovider = PortFileProvider::make_baseline_provider(paths);

Expand Down
2 changes: 2 additions & 0 deletions src/vcpkg/portfileprovider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include <vcpkg/base/system.debug.h>

#include <vcpkg/configuration.h>
#include <vcpkg/metrics.h>
#include <vcpkg/paragraphs.h>
#include <vcpkg/portfileprovider.h>
#include <vcpkg/registries.h>
Expand Down Expand Up @@ -193,6 +194,7 @@ namespace vcpkg::PortFileProvider
}
else
{
Metrics::g_metrics.lock()->track_property("versioning-error-version", "defined");
Copy link
Contributor

@ras0219 ras0219 Mar 3, 2021

Choose a reason for hiding this comment

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

This is a bit imprecise because we don't actually know whether this is a termination-worthy error, however it may be intractable to move this to every load_control_file().value_or_exit().

Copy link
Contributor

@ras0219 ras0219 Mar 3, 2021

Choose a reason for hiding this comment

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

The more invasive but "correct" approach would be to create a new type like:

struct TrackedError {
    std::string message;
    std::string metric;
};

which upon calling Expected<T,TrackedError>::value_or_exit() would track the property just in time to exit. However, I think that is probably too invasive at this time.

Copy link
Member Author

@vicroms vicroms Mar 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I may attempt to track in value_or_exit() in a separate PR.
For the time being, I'll just merge with the current metrics if there are no objections by end of day.

return maybe_path.error();
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/vcpkg/sourceparagraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <vcpkg/base/system.print.h>
#include <vcpkg/base/util.h>

#include <vcpkg/metrics.h>
#include <vcpkg/packagespec.h>
#include <vcpkg/platform-expression.h>
#include <vcpkg/sourceparagraph.h>
Expand Down Expand Up @@ -1033,6 +1034,7 @@ namespace vcpkg
{
if (dep.constraint.type != Versions::Constraint::Type::None)
{
Metrics::g_metrics.lock()->track_property("error-versioning-disabled", "defined");
return Strings::concat(
fs::u8string(origin),
" was rejected because it uses constraints and the `",
Expand All @@ -1053,6 +1055,7 @@ namespace vcpkg

if (core_paragraph->overrides.size() != 0)
{
Metrics::g_metrics.lock()->track_property("error-versioning-disabled", "defined");
return Strings::concat(fs::u8string(origin),
" was rejected because it uses overrides and the `",
VcpkgCmdArguments::VERSIONS_FEATURE,
Expand All @@ -1062,6 +1065,7 @@ namespace vcpkg

if (core_paragraph->builtin_baseline.has_value())
{
Metrics::g_metrics.lock()->track_property("error-versioning-disabled", "defined");
return Strings::concat(
fs::u8string(origin),
" was rejected because it uses builtin-baseline and the `",
Expand All @@ -1080,6 +1084,7 @@ namespace vcpkg
return dependency.constraint.type != Versions::Constraint::Type::None;
}))
{
Metrics::g_metrics.lock()->track_property("error-versioning-no-baseline", "defined");
return Strings::concat(
fs::u8string(origin),
" was rejected because it uses \"version>=\" without setting a \"builtin-baseline\".\n",
Expand All @@ -1088,6 +1093,7 @@ namespace vcpkg

if (!core_paragraph->overrides.empty())
{
Metrics::g_metrics.lock()->track_property("error-versioning-no-baseline", "defined");
return Strings::concat(
fs::u8string(origin),
" was rejected because it uses \"overrides\" without setting a \"builtin-baseline\".\n",
Expand Down