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

src/AppInstallerCommonCore/Experiment.cpp

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.

src/AppInstallerRepositoryCore/SourceList.cpp

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.

src/AppInstallerCommonCore/AppInstallerTelemetry.cpp

src/AppInstallerCommonCore/Experiment.cpp

src/AppInstallerCommonCore/UserSettings.cpp

src/AppInstallerRepositoryCore/SourceList.cpp

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.

src/AppInstallerCommonCore/Experiment.cpp

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?

florelis · 2025-01-03T23:43:54Z

schemas/JSON/settings/settings.schema.0.2.json

+      "type": "object",
+      "properties": {
+        "CDN": {
+          "description": "Enables the use of an alternative CDN for package downloads",


Packages are not downloaded from our CDN AFAIK

Suggested change

"description": "Enables the use of an alternative CDN for package downloads",

"description": "Enables the use of an alternative CDN for the winget source",

florelis · 2025-01-04T00:21:05Z

src/AppInstallerCommonCore/Public/winget/Experiment.h

+        ExperimentState() = default;
+        ExperimentState(bool isEnabled, ExperimentToggleSource toggleSource) : m_isEnabled(isEnabled), m_toggleSource(toggleSource) {}
+
+        /// <summary>


We don't use this doc string format in C++. I think we just add the comment above.

florelis · 2025-01-04T00:35:42Z

src/AppInstallerCLITests/Experiment.cpp

+{
+    SECTION("Not configured")
+    {
+        Experiment::ResetStates();


I would prefer this to be a struct that resets the state on destruction, like GroupPolicyTestOverride and TestUserSettings. Just to be sure that nothing we set here can escape and affect other tests

florelis · 2025-01-04T00:40:57Z

src/AppInstallerCLITests/Experiment.cpp

A couple of extra tests that would be good:

Policy=Disabled and Global Allow=True -> disabled

Policy=Enabled and Global Allow=False -> disabled

Policy=Enabled and Individual Allow=False-> Disabled

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 4 commits December 17, 2024 11:08

Addressing comments

f26ae41

Include dir

0de0174

Added allowExperiments

e1bafb4

Addressing comments

7e12f7f

florelis reviewed Jan 4, 2025

View reviewed changes

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 10, 2024

denelon Dec 10, 2024

JohnMcPMS Dec 10, 2024

JohnMcPMS Dec 10, 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

florelis Jan 3, 2025

florelis Jan 4, 2025

florelis Jan 4, 2025

florelis Jan 4, 2025

	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; }

	"description": "Enables the use of an alternative CDN for package downloads",
	"description": "Enables the use of an alternative CDN for the winget source",

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

AmelBawa-msft commented Dec 3, 2024 •

edited by microsoft-github-policy-service bot

Loading