Adding the client code for supporting experiments #5034

AmelBawa-msft · 2024-12-03T22:27:22Z

Adding the client-side code for supporting running experiments in winget-cli.

Group policy:

EnableExperiments: Disabling this group policy will prevent any experiment from running.

User settings:

"experiments": Enable the user to control whether or not an experiment should run or not.
I have signed the Contributor License Agreement.
This pull request is related to an issue.

Microsoft Reviewers: Open in CodeFlow

AmelBawa-msft · 2024-12-09T21:36:55Z

doc/admx/en-US/DesktopAppInstaller.adml

@@ -25,6 +25,12 @@ If you disable this setting, users will not be able to change settings for the W
 If you enable or do not configure this setting, users will be able to enable experimental features for the Windows Package Manager.

 If you disable this setting, users will not be able to enable experimental features for the Windows Package Manager.</string>
+      <string id="EnableExperiments">Enable Windows Package Manager Experiments</string>


Adding @RDMacLachlan to help with the strings to be used in the adml file

src/Internal/Experiment/Experiment.vcxproj

src/Internal/Experiment/Experiment.h

florelis · 2024-12-09T22:42:24Z

src/AppInstallerCommonCore/Experiment.cpp

+                AICLI_LOG(Core, Info, <<
+                    "Experiment " << Experiment::GetExperiment(key).Name() <<
+                    " is disabled due to group policy: " << TogglePolicy::GetPolicy(TogglePolicy::Policy::Experiments).RegValueName());
+                return { false, ExperimentToggleSource::Policy };


It looks like you're intending for all experiments to have a default value of "disabled". I think the API we'll use allows us to have the control group be "enabled" and the experiment "disabled". We should at least add a comment somewhere we'll see while adding a new experiment to remind us to not add experiments like that

Could you please provide more details about this comment? I’m not entirely clear on what the comment should mention.

Maybe we should consider the meaning of the boolean associated with an experiment not "enabled" or "disabled", but rather, "In the experimental group?" That makes the default state of "false" mean "Not in the experimental group", and thus more directly maps to group policy that disables running experiments. A comment on ExperimentState::IsEnabled indicating this semantic and counter-indicating use of the "inverted" experiment would be sufficient.

florelis · 2024-12-09T22:43:52Z

doc/admx/en-US/DesktopAppInstaller.adml

@@ -25,6 +25,12 @@ If you disable this setting, users will not be able to change settings for the W
 If you enable or do not configure this setting, users will be able to enable experimental features for the Windows Package Manager.

 If you disable this setting, users will not be able to enable experimental features for the Windows Package Manager.</string>
+      <string id="EnableExperiments">Enable Windows Package Manager Experiments</string>
+      <string id="EnableExperimentsExplanation">This policy controls whether users can enable experiments in the Windows Package Manager.


We may need to expand on what an experiment is, and how it differs from an experimental feature, so that IT admins can make the decision

+1 this is a placeholder text I added and will be replaced by PM's suggestion 😊

I would use "Experimentation" rather than "Experiments"

The policy controls whether users can have experiments run in WinGet on their device.

Does the policy control:

experimentation all up

experiment state being driven externally

user control over experiment state

This string seems to indicate only 3, but my expectation would be either 1 or 2.

florelis · 2024-12-09T23:26:05Z

src/AppInstallerRepositoryCore/SourceList.cpp

+        std::string_view GetWingetCommunitySource()
+        {
+            if (Settings::Experiment::GetState(Settings::Experiment::Key::CDN).IsEnabled())
+            {
+                return s_Source_WingetCommunityExperimental_Arg;
+            }
+
+            return s_Source_WingetCommunityDefault_Arg;
+        }


We do some validation when a user tries to add one of the default sources. For example, to block it if the group policy restricts the default source. This may allow for an unintended way to bypass that validation. E.g., somebody has the experiment and the group policy, which means we block adding the cdn2.winget... source, but they can still add cdn.winget...

Yes, I think the following should cover it:

Remove GetWellKnownSourceArg

Update CheckForWellKnownSourceMatch to
a. be case insensitive
b. handle both DNS entries
c. allow for not checking certain values (ex. name), maybe by passing an empty string_view

Replace use of GetWellKnownSourceArg in GetPolicyBlockingUserSource with the updated CheckForWellKnownSourceMatch

It didn't look like any other paths were relevant there.

florelis · 2024-12-09T23:27:50Z

src/AppInstallerRepositoryCore/SourceList.cpp

@@ -278,7 +290,7 @@ namespace AppInstaller::Repository

    std::optional<WellKnownSource> CheckForWellKnownSourceMatch(std::string_view name, std::string_view arg, std::string_view type)
    {
-        if (name == s_Source_WingetCommunityDefault_Name && arg == s_Source_WingetCommunityDefault_Arg && type == Microsoft::PreIndexedPackageSourceFactory::Type())
+        if (name == s_Source_WingetCommunityDefault_Name && arg == GetWingetCommunitySource() && type == Microsoft::PreIndexedPackageSourceFactory::Type())


See my previous comment. Not sure if this is the only place that needs something like this

Suggested change

if (name == s_Source_WingetCommunityDefault_Name && arg == GetWingetCommunitySource() && type == Microsoft::PreIndexedPackageSourceFactory::Type())

if (name == s_Source_WingetCommunityDefault_Name && (arg == s_Source_WingetCommunityDefault_Arg || arg == s_Source_WingetCommunityExperimental_Arg) && type == Microsoft::PreIndexedPackageSourceFactory::Type())

src/AppInstallerCommonCore/Experiment.cpp

src/AppInstallerCommonCore/Public/winget/Experiment.h

src/AppInstallerCommonCore/Experiment.cpp

JohnMcPMS · 2024-12-10T22:18:16Z

doc/admx/en-US/DesktopAppInstaller.adml

@@ -25,6 +25,12 @@ If you disable this setting, users will not be able to change settings for the W
 If you enable or do not configure this setting, users will be able to enable experimental features for the Windows Package Manager.

 If you disable this setting, users will not be able to enable experimental features for the Windows Package Manager.</string>
+      <string id="EnableExperiments">Enable Windows Package Manager Experiments</string>
+      <string id="EnableExperimentsExplanation">This policy controls whether users can enable experiments in the Windows Package Manager.


Does the policy control:

experimentation all up

experiment state being driven externally

user control over experiment state

This string seems to indicate only 3, but my expectation would be either 1 or 2.

JohnMcPMS · 2024-12-10T22:41:38Z

src/AppInstallerCommonCore/AppInstallerTelemetry.cpp

@@ -11,19 +11,22 @@

 #define AICLI_TraceLoggingStringView(_sv_,_name_) TraceLoggingCountedUtf8String(_sv_.data(), static_cast<ULONG>(_sv_.size()), _name_)
 #define AICLI_TraceLoggingWStringView(_sv_,_name_) TraceLoggingCountedWideString(_sv_.data(), static_cast<ULONG>(_sv_.size()), _name_)
+#define AICLI_TraceLoggingJsonWString(_sv_, _name_) TraceLoggingPackedFieldEx(_sv_.c_str(), static_cast<ULONG>((_sv_.size() + 1) * sizeof(wchar_t)), TlgInUNICODESTRING, TlgOutJSON, _name_)
+#define AICLI_TraceLoggingJsonString(_sv_, _name_) TraceLoggingPackedFieldEx(_sv_.c_str(), static_cast<ULONG>((_sv_.size() + 1) * sizeof(char)), TlgInUNICODESTRING, TlgOutJSON, _name_)


There are several things wrong with this:

It should be TlgInANSISTRING for 1 byte characters

It should be TlgOutUTF8 for UTF-8 encoding

It can't be both TlgOutUTF8 and TlgOutJSON as they are not flags

So ultimately you need to just get rid of it and force conversion to UTF-16 and use of AICLI_TraceLoggingJsonWString.

JohnMcPMS · 2024-12-10T22:44:45Z

src/AppInstallerCommonCore/AppInstallerTelemetry.cpp

+    Settings::ExperimentState TelemetryTraceLogger::GetExperimentState(Experiment::Key key)
+    {
+#ifndef AICLI_DISABLE_TEST_HOOKS
+        m_summary.Experiments.clear();


So we will never be in a state of having two experiments during testing? Make an explicit test hook function to clear it rather than always doing it.

src/AppInstallerCommonCore/AppInstallerTelemetry.cpp

JohnMcPMS · 2024-12-10T22:48:57Z

src/AppInstallerCommonCore/AppInstallerTelemetry.cpp

@@ -77,6 +80,19 @@ namespace AppInstaller::Logging
                return "unknown"sv;
            }
        }
+
+        std::string GetExperimentsJson(const std::map<Experiment::Key, ExperimentState>& experiments)


Please create an ExperimentStateCache so that this code and the lookup can live in Experiment.cpp.

src/AppInstallerCommonCore/Experiment.cpp

src/AppInstallerCommonCore/UserSettings.cpp

JohnMcPMS · 2024-12-11T00:08:59Z

src/AppInstallerRepositoryCore/SourceList.cpp

+        std::string_view GetWingetCommunitySource()
+        {
+            if (Settings::Experiment::GetState(Settings::Experiment::Key::CDN).IsEnabled())
+            {
+                return s_Source_WingetCommunityExperimental_Arg;
+            }
+
+            return s_Source_WingetCommunityDefault_Arg;
+        }


Yes, I think the following should cover it:

Remove GetWellKnownSourceArg

Update CheckForWellKnownSourceMatch to
a. be case insensitive
b. handle both DNS entries
c. allow for not checking certain values (ex. name), maybe by passing an empty string_view

Replace use of GetWellKnownSourceArg in GetPolicyBlockingUserSource with the updated CheckForWellKnownSourceMatch

It didn't look like any other paths were relevant there.

JohnMcPMS · 2024-12-11T00:10:17Z

src/AppInstallerSharedLib/JsonUtil.cpp

+    template<>
+    std::optional<std::map<std::string, bool>> GetValue(const Json::Value& node)
+    {
+        std::map<std::string, bool> result;


Move into inner scope.

src/AppInstallerSharedLib/JsonUtil.cpp

florelis · 2024-12-11T19:15:34Z

doc/admx/DesktopAppInstaller.admx

I imagine there is already a policy to disable experimentation across Windows as a whole. I found AllowExperimentation but I'm not sure if it does what I think.

If there is such a policy, I think we would be better off not having our own and just pointing people to that one. If somebody disables it system-wide, then our policy won't do anything because the infrastructure we use will be disabled. The only case where it would be useful to have both is if somebody wants to have experimentation in Windows as a whole, but not on winget specifically, which I don't think is a common scenario

florelis · 2024-12-11T19:19:15Z

doc/admx/en-US/DesktopAppInstaller.adml

@@ -25,6 +25,12 @@ If you disable this setting, users will not be able to change settings for the W
 If you enable or do not configure this setting, users will be able to enable experimental features for the Windows Package Manager.

 If you disable this setting, users will not be able to enable experimental features for the Windows Package Manager.</string>
+      <string id="EnableExperiments">Enable Windows Package Manager Experiments</string>


I would suggest the policy be called "Allow" instead of "Enable" even if it is not consistent with the other policies.
To me, "Enable" means that the experiments will be turned on but that's not what happens. What happens is that experiments may be turned on depending on the control/treatment groups; which is better described by "Allow"

It probably should also mention that they are Microsoft-run experiments for new features/changes

florelis · 2024-12-11T20:08:40Z

src/AppInstallerCommonCore/Public/winget/Experiment.h

+        bool m_isEnabled;
+    };
+
+    struct Experiment


Could we make experiments part of the experimental features?

Experimental features already have most of the things we may want: settings to be turned on/off, GPO to disable them wholesale, an API to check the state, a command to show the current state. The only thing we would need to add is the option that they may be set remotely for an experiment if there is no local override.

I think we might still want a new GP for "allow experimental feature state to be changed remotely", but other than that it was my original thought that all experiments would likely have a feature. I forget the reasons why I softened on that, but I do think that there may be some value to separating them as they are slightly different semantics. One potential large reason is that if we use EFs exclusively, then a disabled GP for EFs also disables experiments. Moving experiments out to their own thing means an admin can disable in-development features while still allowing for experiments.

Typically, an EF is work-in-progress and will eventually be released. Using an experiment on an EF is more akin to controlled feature rollout. I think it will probably happen that we hook that up so that an EF can be controlled by an experiment and thus we get CFR.

On the other hand, a true experiment is typically a small configuration or code change that is itself not risky from a local functionality perspective (the impact, such as changing the CDN could have major impacts on the overall system of course). We might be testing something that we want to make permanent like where the DNS for the CDN points, or we might be checking to see if customer satisfaction numbers are higher if the default progress style is rainbow.

as they are slightly different semantics

If users are going to come across the two concepts, do we think the difference is clear enough for them to understand?

an admin can disable in-development features while still allowing for experiments

Do we expect there will be admins wanting to do that?

I don't think we want users to come across experiments really; the major reason for the control via settings is to enable us to test.

As to admins wanting to do that; I'm not sure. But if we are not going to build any user facing interfaces around them, then I see the minor additional cost as acceptable.

JohnMcPMS · 2024-12-12T18:03:23Z

src/AppInstallerCommonCore/Experiment.cpp

                    " is disabled due to group policy: " << TogglePolicy::GetPolicy(TogglePolicy::Policy::Experiments).RegValueName());
                return { false, ExperimentToggleSource::Policy };
            }

-            auto experiments = userSettings.Get<Setting::Experiments>();
+            if (key == Experiment::Key::None)


Move this back to first. While I'm not sure that we need a None, if we have one then checks against it should not end up logging about group policy being disabled.

JohnMcPMS · 2024-12-12T18:05:01Z

src/AppInstallerCommonCore/Experiment.cpp

+            }
+
+            auto experiments = userSettings.Get<Setting::Experiments>();
+            auto experiment = Experiment::GetExperiment(key);


It looks like you did this, but only to uses after the existing experiment local was created. Move the local earlier so that it can be truly constructed once.

JohnMcPMS · 2024-12-12T18:10:16Z

src/AppInstallerCommonCore/Public/winget/Experiment.h

-        Experiment(std::string_view name, std::string_view jsonName, std::string_view link, std::string key) :
-            m_name(name), m_jsonName(jsonName), m_link(link), m_key(key) {}
+        Experiment(std::string name, std::string jsonName, std::string link, std::string key) :
+            m_name(std::move(name)), m_jsonName(jsonName), m_link(std::move(link)), m_key(std::move((key))) {}


Suggested change

m_name(std::move(name)), m_jsonName(jsonName), m_link(std::move(link)), m_key(std::move((key))) {}

m_name(std::move(name)), m_jsonName(std::move(jsonName)), m_link(std::move(link)), m_key(std::move((key))) {}

Accompanied by making m_jsonName not a view.

JohnMcPMS · 2024-12-12T18:10:36Z

src/AppInstallerCommonCore/Public/winget/Experiment.h


        static ExperimentState GetState(Key feature);
        static ExperimentState GetStateInternal(Key feature);
        static Experiment GetExperiment(Key key);
        static std::vector<Experiment> GetAllExperiments();

-        std::string_view Name() const { return m_name; }
+        std::string Name() const { return m_name; }


Suggested change

std::string Name() const { return m_name; }

const std::string& Name() const { return m_name; }

And the rest as const&.

JohnMcPMS · 2024-12-12T18:14:09Z

src/AppInstallerCommonCore/Public/winget/Experiment.h

        std::string GetKey() const { return m_key; }

    private:
-        std::string_view m_name;
+        std::string m_name;
        Utility::LocIndView m_jsonName;


This does not need to be a localized value, does it?

AmelBawa-msft added 9 commits November 7, 2024 13:56

Init files

20d1d66

Init experiment

3e66324

Add template

e9d77e9

Added logs

f95c26c

Add CDN exp

1d7fcd0

Added UT

1720e20

Store the toggle source

2072721

draft

9a41fff

Addressed offline comments

ff0ac7b

AmelBawa-msft marked this pull request as ready for review December 9, 2024 21:26

AmelBawa-msft requested a review from a team as a code owner December 9, 2024 21:26

This was linked to issues Dec 9, 2024

(GPO) Group Policy Object - Enable Experimentation #4896

Open

Support experimentation #4364

Open

AmelBawa-msft commented Dec 9, 2024

View reviewed changes

florelis reviewed Dec 9, 2024

View reviewed changes

Resolved conflicts

9dbc7d4

microsoft-github-policy-service bot added the Issue-Feature This is a feature request for the Windows Package Manager client. label Dec 10, 2024

Addressing comments

bdf8c90

JohnMcPMS reviewed Dec 11, 2024

View reviewed changes

florelis reviewed Dec 11, 2024

View reviewed changes

Addressing comments

dee65a9

JohnMcPMS reviewed Dec 12, 2024

View reviewed changes

AmelBawa-msft added 3 commits December 17, 2024 11:08

Addressing comments

f26ae41

Include dir

0de0174

Added allowExperiments

e1bafb4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the client code for supporting experiments #5034

Adding the client code for supporting experiments #5034

AmelBawa-msft commented Dec 3, 2024 •

edited by microsoft-github-policy-service bot

Loading

AmelBawa-msft Dec 9, 2024

florelis Dec 9, 2024

AmelBawa-msft Dec 11, 2024

JohnMcPMS Dec 12, 2024

florelis Dec 9, 2024

AmelBawa-msft Dec 10, 2024

denelon Dec 10, 2024

JohnMcPMS Dec 10, 2024

florelis Dec 9, 2024

JohnMcPMS Dec 11, 2024

florelis Dec 9, 2024

JohnMcPMS Dec 10, 2024

JohnMcPMS Dec 10, 2024

JohnMcPMS Dec 10, 2024

JohnMcPMS Dec 10, 2024

JohnMcPMS Dec 11, 2024

JohnMcPMS Dec 11, 2024

florelis Dec 11, 2024

florelis Dec 11, 2024

florelis Dec 11, 2024

JohnMcPMS Dec 12, 2024

florelis Dec 12, 2024

JohnMcPMS Dec 12, 2024

JohnMcPMS Dec 12, 2024

JohnMcPMS Dec 12, 2024

JohnMcPMS Dec 12, 2024

JohnMcPMS Dec 12, 2024

JohnMcPMS Dec 12, 2024

	if (name == s_Source_WingetCommunityDefault_Name && arg == GetWingetCommunitySource() && type == Microsoft::PreIndexedPackageSourceFactory::Type())
	if (name == s_Source_WingetCommunityDefault_Name && (arg == s_Source_WingetCommunityDefault_Arg \|\| arg == s_Source_WingetCommunityExperimental_Arg) && type == Microsoft::PreIndexedPackageSourceFactory::Type())

	m_name(std::move(name)), m_jsonName(jsonName), m_link(std::move(link)), m_key(std::move((key))) {}
	m_name(std::move(name)), m_jsonName(std::move(jsonName)), m_link(std::move(link)), m_key(std::move((key))) {}

	std::string Name() const { return m_name; }
	const std::string& Name() const { return m_name; }

Adding the client code for supporting experiments #5034

Are you sure you want to change the base?

Adding the client code for supporting experiments #5034

Conversation

AmelBawa-msft commented Dec 3, 2024 • edited by microsoft-github-policy-service bot Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmelBawa-msft commented Dec 3, 2024 •

edited by microsoft-github-policy-service bot

Loading