welcome_tour: Add synthetic field [chromium/src : main]
Tao Wu (Gerrit)
Tao Wu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tao Wu
| Commit-Queue | +1 |
1 comment
Related details
- David Black
- Li Lin
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
mpea...@chromium.org is from context(analysis/uma/chrome-metrics.gwsq)
Related details
- David Black
- Li Lin
- Mark Pearson
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Black (Gerrit)
David Black added 14 comments![]( "Open in Gerrit")
(const std::string& trail_name, const std::string& group_name),nit: typo :)
void RegisterSyntheticFieldTrial();Let's add a passkey since we don't intend to make this API canonical and we'll remove it once the Welcome Tour experiment is completed :)
// Register sythetical field trial.nit: typo :)
constexpr char kTrialName[] = "CrOSWelcomeTour";Can we use a name that aligns w/ the non-synthetic study so it is more obvious that they relate to each other? :)
// TODO: b/345829923 - Set up `delegate_` in the test base. group_name = base::StrCat({kGroupNamePrefix, "V1"});
} else if (features::IsWelcomeTourHoldbackEnabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourV2Enabled());
group_name = base::StrCat({kGroupNamePrefix, "Holdback"});
} else if (features::IsWelcomeTourV2Enabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourHoldbackEnabled());
group_name = base::StrCat({kGroupNamePrefix, "V2"});Can we use the same arm names for the synthetic experiment arms as we do for the non-synthetic experiment arms for consistency? :)
std::string group_name;
if (features::IsWelcomeTourCounterfactuallyEnabled()) {
CHECK(!features::IsWelcomeTourHoldbackEnabled());
CHECK(!features::IsWelcomeTourV2Enabled());
group_name = base::StrCat({kGroupNamePrefix, "V1"});
} else if (features::IsWelcomeTourHoldbackEnabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourV2Enabled());
group_name = base::StrCat({kGroupNamePrefix, "Holdback"});
} else if (features::IsWelcomeTourV2Enabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourHoldbackEnabled());
group_name = base::StrCat({kGroupNamePrefix, "V2"});
}
if (!group_name.empty()) {
delegate_->RegisterSyntheticFieldTrial(kTrialName, group_name);
}The UserEducationController is framework level code and shouldn't contain feature level logic :) The trial and group names pertaining to the Welcome Tour should be passed in from the WelcomeTourController :)
virtual void RegisterSyntheticFieldTrial(const std::string& trial_name,
const std::string& group_name) = 0;Let's use string_view to avoid creating unnecessary strings :)
// Register sythetical field trial.nit: typo :)
// Set expectations for whether the synthetic field trail will be registered.nit: typo :)
// Set expectations for whether the synthetic field trail will be registered.
const bool should_register = features::IsWelcomeTourV2Enabled() ||
features::IsWelcomeTourCounterfactuallyEnabled();
std::string arm = features::IsWelcomeTourCounterfactuallyEnabled()
? "ExperimentalArm_V1"
: features::IsWelcomeTourV2Enabled() ? "ExperimentalArm_V2"
: "";
EXPECT_CALL(*user_education_delegate(),
RegisterSyntheticFieldTrial(Eq("CrOSWelcomeTour"), Eq(arm)))
.Times(should_register ? 1u : 0u);This doesn't verify that some unexpected synthetic field trial isn't registered :) Maybe something like:
```
std::string experimental_name;
if (features::IsWelcomeTourCounterfactuallyEnabled()) {
experimental_name = "V1";
} else if (features::IsWelcomeTourV2Enabled()) {
experimental_name = "V2";
}
EXPECT_CALL(*user_education_delegate(),
RegisterSyntheticFieldTrial(
Conditional(experimental_name.size(), "CrOSWelcomeTour", Any()),
Conditional(experimental_name.size(), experimental_name, Any())))
.Times(experimental_name.size() ? 1u : 0u);
```
// Set expectations for whether the synthetic field trail will be
// registered.
EXPECT_CALL(*user_education_delegate(),
RegisterSyntheticFieldTrial(Eq("CrOSWelcomeTour"),
Eq("ExperimentalArm_Holdback")))
.Times(IsHoldback().value_or(false) ? 1u : 0u);Similar to above, maybe something like:
```
std::string experimental_name;
if (features::IsWelcomeTourHoldback()) {
experimental_name = "Holdback";
}
EXPECT_CALL(*user_education_delegate(),
RegisterSyntheticFieldTrial(
Conditional(experimental_name.size(), "CrOSWelcomeTour", Any()),
Conditional(experimental_name.size(), experimental_name, Any())))
.Times(experimental_name.size() ? 1u : 0u);
```
Related details
- Li Lin
- Mark Pearson
- Tao Wu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tao Wu (Gerrit)
Tao Wu voted and added 15 comments![]( "Open in Gerrit")
Votes added by Tao Wu
| Commit-Queue | +1 |
15 comments
(const std::string& trail_name, const std::string& group_name),Tao Wunit: typo :)
Done
Let's add a passkey since we don't intend to make this API canonical and we'll remove it once the Welcome Tour experiment is completed :)
Do you ask to add params?
RegisterSyntheticFieldTrial(string_view, string_view)?
// Register sythetical field trial.Tao Wunit: typo :)
Done
Can we use a name that aligns w/ the non-synthetic study so it is more obvious that they relate to each other? :)
How about ChromeOSWelcomeTourSyntheticStudy?
// TODO: b/345829923 - Set up `delegate_` in the test base. group_name = base::StrCat({kGroupNamePrefix, "V1"});
} else if (features::IsWelcomeTourHoldbackEnabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourV2Enabled());
group_name = base::StrCat({kGroupNamePrefix, "Holdback"});
} else if (features::IsWelcomeTourV2Enabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourHoldbackEnabled());
group_name = base::StrCat({kGroupNamePrefix, "V2"});Can we use the same arm names for the synthetic experiment arms as we do for the non-synthetic experiment arms for consistency? :)
We can use `Counterfactual`, `Holdback`, and `Enabled`
std::string group_name;
if (features::IsWelcomeTourCounterfactuallyEnabled()) {
CHECK(!features::IsWelcomeTourHoldbackEnabled());
CHECK(!features::IsWelcomeTourV2Enabled());
group_name = base::StrCat({kGroupNamePrefix, "V1"});
} else if (features::IsWelcomeTourHoldbackEnabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourV2Enabled());
group_name = base::StrCat({kGroupNamePrefix, "Holdback"});
} else if (features::IsWelcomeTourV2Enabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourHoldbackEnabled());
group_name = base::StrCat({kGroupNamePrefix, "V2"});
}
if (!group_name.empty()) {
delegate_->RegisterSyntheticFieldTrial(kTrialName, group_name);
}The UserEducationController is framework level code and shouldn't contain feature level logic :) The trial and group names pertaining to the Welcome Tour should be passed in from the WelcomeTourController :)
Done
virtual void RegisterSyntheticFieldTrial(const std::string& trial_name,
const std::string& group_name) = 0;Let's use string_view to avoid creating unnecessary strings :)
Done
// Register sythetical field trial.Tao Wunit: typo :)
Done
// Set expectations for whether the synthetic field trail will be registered.Tao Wunit: typo :)
Done
// Set expectations for whether the synthetic field trail will be registered.
const bool should_register = features::IsWelcomeTourV2Enabled() ||
features::IsWelcomeTourCounterfactuallyEnabled();
std::string arm = features::IsWelcomeTourCounterfactuallyEnabled()
? "ExperimentalArm_V1"
: features::IsWelcomeTourV2Enabled() ? "ExperimentalArm_V2"
: "";
EXPECT_CALL(*user_education_delegate(),
RegisterSyntheticFieldTrial(Eq("CrOSWelcomeTour"), Eq(arm)))
.Times(should_register ? 1u : 0u);This doesn't verify that some unexpected synthetic field trial isn't registered :) Maybe something like:
```
std::string experimental_name;
if (features::IsWelcomeTourCounterfactuallyEnabled()) {
experimental_name = "V1";
} else if (features::IsWelcomeTourV2Enabled()) {
experimental_name = "V2";
}EXPECT_CALL(*user_education_delegate(),
RegisterSyntheticFieldTrial(
Conditional(experimental_name.size(), "CrOSWelcomeTour", Any()),
Conditional(experimental_name.size(), experimental_name, Any())))
.Times(experimental_name.size() ? 1u : 0u);
```
Done. I cannot tell what the difference is 😞 What's the unexpected synthetic field trial?
// Set expectations for whether the synthetic field trail will be
// registered.
EXPECT_CALL(*user_education_delegate(),
RegisterSyntheticFieldTrial(Eq("CrOSWelcomeTour"),
Eq("ExperimentalArm_Holdback")))
.Times(IsHoldback().value_or(false) ? 1u : 0u);Similar to above, maybe something like:
```
std::string experimental_name;
if (features::IsWelcomeTourHoldback()) {
experimental_name = "Holdback";
}EXPECT_CALL(*user_education_delegate(),
RegisterSyntheticFieldTrial(
Conditional(experimental_name.size(), "CrOSWelcomeTour", Any()),
Conditional(experimental_name.size(), experimental_name, Any())))
.Times(experimental_name.size() ? 1u : 0u);
```
Done
Related details
- David Black
- Li Lin
- Mark Pearson
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tao Wu (Gerrit)
Tao Wu added 1 comment![]( "Open in Gerrit")
// Set expectations for whether the synthetic field trail will be registered.
const bool should_register = features::IsWelcomeTourV2Enabled() ||
features::IsWelcomeTourCounterfactuallyEnabled();
std::string arm = features::IsWelcomeTourCounterfactuallyEnabled()
? "ExperimentalArm_V1"
: features::IsWelcomeTourV2Enabled() ? "ExperimentalArm_V2"
: "";
EXPECT_CALL(*user_education_delegate(),
RegisterSyntheticFieldTrial(Eq("CrOSWelcomeTour"), Eq(arm)))
.Times(should_register ? 1u : 0u);Tao WuThis doesn't verify that some unexpected synthetic field trial isn't registered :) Maybe something like:
```
std::string experimental_name;
if (features::IsWelcomeTourCounterfactuallyEnabled()) {
experimental_name = "V1";
} else if (features::IsWelcomeTourV2Enabled()) {
experimental_name = "V2";
}EXPECT_CALL(*user_education_delegate(),
RegisterSyntheticFieldTrial(
Conditional(experimental_name.size(), "CrOSWelcomeTour", Any()),
Conditional(experimental_name.size(), experimental_name, Any())))
.Times(experimental_name.size() ? 1u : 0u);
```
Done. I cannot tell what the difference is 😞 What's the unexpected synthetic field trial?
I got it! Good way!
Related details
- David Black
- Li Lin
- Mark Pearson
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Black (Gerrit)
David Black added 12 comments![]( "Open in Gerrit")
// Register synthetical field trial.nit: synthetic :)
void RegisterSyntheticFieldTrial();Tao WuLet's add a passkey since we don't intend to make this API canonical and we'll remove it once the Welcome Tour experiment is completed :)
Do you ask to add params?
RegisterSyntheticFieldTrial(string_view, string_view)?
Yes, looks good :)
constexpr char kTrialName[] = "CrOSWelcomeTour";Tao WuCan we use a name that aligns w/ the non-synthetic study so it is more obvious that they relate to each other? :)
How about ChromeOSWelcomeTourSyntheticStudy?
The finch study is "ChromeOSWelcomeTourV2", so I would do something like "ChromeOSWelcomeTourV2_Synthetic" for the associated synthetic field trial so it is more obvious that the two are related and so that a search for "ChromeOSWelcomeTourV2" will match both :)
group_name = base::StrCat({kGroupNamePrefix, "V1"});
} else if (features::IsWelcomeTourHoldbackEnabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourV2Enabled());
group_name = base::StrCat({kGroupNamePrefix, "Holdback"});
} else if (features::IsWelcomeTourV2Enabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourHoldbackEnabled());
group_name = base::StrCat({kGroupNamePrefix, "V2"});Tao WuCan we use the same arm names for the synthetic experiment arms as we do for the non-synthetic experiment arms for consistency? :)
We can use `Counterfactual`, `Holdback`, and `Enabled`
Sgtm :)
// Register synthetical field trial.nit: synthetic :)
constexpr char kGroupNamePrefix[] = "ExperimentalArm_";Is this prefix necessary or can the arms just be "Enabled", "Counterfactual" and "Holdback" like they are in the finch study? :)
std::string GetSyntheticStudyGroupName() {nit: Use `std::optional` to express that this API may return an empty value, otherwise it is non-obvious from the method signature :)
std::string GetSyntheticStudyGroupName() {
std::string group_name;
if (features::IsWelcomeTourCounterfactuallyEnabled()) {
CHECK(!features::IsWelcomeTourHoldbackEnabled());
CHECK(!features::IsWelcomeTourV2Enabled());
group_name = base::StrCat({kGroupNamePrefix, "Counterfactual"});
} else if (features::IsWelcomeTourHoldbackEnabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourV2Enabled());
group_name = base::StrCat({kGroupNamePrefix, "Holdback"});
} else if (features::IsWelcomeTourV2Enabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourHoldbackEnabled());
group_name = base::StrCat({kGroupNamePrefix, "Enabled"});
}
return group_name;
}nit: sort w/ other helper methods :)
const auto group_name = GetSyntheticStudyGroupName();
if (!group_name.empty()) {nit: inline :)
```suggestion
if (const auto group_name = GetSyntheticStudyGroupName(); !group_name.empty()) {
```
welcome_tour_metrics::RecordExperimentalArm();nit: newline above :)
👍 This way we ensure that "Holdback", for example, isn't mistakenly registered :)
"ChromeOSWelcomeTourSyntheticStudy", testing::_),nit: just `_`, same for the three instances below :)
Related details
- Li Lin
- Mark Pearson
- Tao Wu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Black (Gerrit)
David Black added 1 comment![]( "Open in Gerrit")
void RegisterSyntheticFieldTrial();Tao WuLet's add a passkey since we don't intend to make this API canonical and we'll remove it once the Welcome Tour experiment is completed :)
David BlackDo you ask to add params?
RegisterSyntheticFieldTrial(string_view, string_view)?
Yes, looks good :)
Sorry, forgot the passkey :) Should be something like:
```
RegisterSyntheticFieldTrial(
UserEducationPrivateApiKey,
std::string_view trial_name,
std::string_view group_name);
```
Tao Wu (Gerrit)
Tao Wu voted and added 11 comments![]( "Open in Gerrit")
Votes added by Tao Wu
| Commit-Queue | +1 |
11 comments
// Register synthetical field trial.Tao Wunit: synthetic :)
Done
void RegisterSyntheticFieldTrial();Tao WuLet's add a passkey since we don't intend to make this API canonical and we'll remove it once the Welcome Tour experiment is completed :)
David BlackDo you ask to add params?
RegisterSyntheticFieldTrial(string_view, string_view)?
David BlackYes, looks good :)
Sorry, forgot the passkey :) Should be something like:
```
RegisterSyntheticFieldTrial(
UserEducationPrivateApiKey,
std::string_view trial_name,
std::string_view group_name);
```
Done
Did not notice this before.
constexpr char kTrialName[] = "CrOSWelcomeTour";Tao WuCan we use a name that aligns w/ the non-synthetic study so it is more obvious that they relate to each other? :)
David BlackHow about ChromeOSWelcomeTourSyntheticStudy?
The finch study is "ChromeOSWelcomeTourV2", so I would do something like "ChromeOSWelcomeTourV2_Synthetic" for the associated synthetic field trial so it is more obvious that the two are related and so that a search for "ChromeOSWelcomeTourV2" will match both :)
Done
// Register synthetical field trial.Tao Wunit: synthetic :)
Done
Is this prefix necessary or can the arms just be "Enabled", "Counterfactual" and "Holdback" like they are in the finch study? :)
Done
nit: Use `std::optional` to express that this API may return an empty value, otherwise it is non-obvious from the method signature :)
Done
std::string GetSyntheticStudyGroupName() {
std::string group_name;
if (features::IsWelcomeTourCounterfactuallyEnabled()) {
CHECK(!features::IsWelcomeTourHoldbackEnabled());
CHECK(!features::IsWelcomeTourV2Enabled());
group_name = base::StrCat({kGroupNamePrefix, "Counterfactual"});
} else if (features::IsWelcomeTourHoldbackEnabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourV2Enabled());
group_name = base::StrCat({kGroupNamePrefix, "Holdback"});
} else if (features::IsWelcomeTourV2Enabled()) {
CHECK(!features::IsWelcomeTourCounterfactuallyEnabled());
CHECK(!features::IsWelcomeTourHoldbackEnabled());
group_name = base::StrCat({kGroupNamePrefix, "Enabled"});
}
return group_name;
}nit: sort w/ other helper methods :)
Done
const auto group_name = GetSyntheticStudyGroupName();
if (!group_name.empty()) {nit: inline :)
```suggestion
if (const auto group_name = GetSyntheticStudyGroupName(); !group_name.empty()) {
```
Done
welcome_tour_metrics::RecordExperimentalArm();Tao Wunit: newline above :)
Done
nit: just `_`, same for the three instances below :)
- David Black
- Li Lin
- Mark Pearson
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Black (Gerrit)
David Black voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Black
| Code-Review | +1 |
1 comment
- Li Lin
- Mark Pearson
- Tao Wu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Pearson (Gerrit)
Mark Pearson added 1 comment![]( "Open in Gerrit")
Add the synthetic field to record the experimental arm, because this
feature is enabled by default, we need a way to tell if the tour was
attempted after applying the Finch config.
Is there a doc describing this experiment design? I've previously reviewed first-run experiments, both on platforms that support Finch first-run and on platforms that did not. None of the experiments I've reviewed sound like they'd be described in the manner described here. As such, I'd like to better understand what you're doing before I approve.
thanks,
mark
Related details
- Li Lin
- Tao Wu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tao Wu (Gerrit)
Tao Wu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tao Wu
| Commit-Queue | +1 |
1 comment
Add the synthetic field to record the experimental arm, because this
feature is enabled by default, we need a way to tell if the tour was
attempted after applying the Finch config.Is there a doc describing this experiment design? I've previously reviewed first-run experiments, both on platforms that support Finch first-run and on platforms that did not. None of the experiments I've reviewed sound like they'd be described in the manner described here. As such, I'd like to better understand what you're doing before I approve.
thanks,
mark
Hi Mark, I have added you to the doc: go/cros-welcometourv2-dd
Thank you!
Related details
- Li Lin
- Mark Pearson
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tao Wu (Gerrit)
Tao Wu added 1 comment![]( "Open in Gerrit")
Tao WuAdd the synthetic field to record the experimental arm, because this
feature is enabled by default, we need a way to tell if the tour was
attempted after applying the Finch config.Is there a doc describing this experiment design? I've previously reviewed first-run experiments, both on platforms that support Finch first-run and on platforms that did not. None of the experiments I've reviewed sound like they'd be described in the manner described here. As such, I'd like to better understand what you're doing before I approve.
thanks,
mark
Hi Mark, I have added you to the doc: go/cros-welcometourv2-dd
Thank you!
To summarize here:
Because the Welcome Tour V1 feature is enabled by default, the WelcomeTour will show only once as the first run after OOBE. There is a problem that many new users may not reboot and do not apply the Finch config in the first run. Therefore, this could cause problems like this:
When the user receives the V2 (or other) treatment it will be too late: N% of those users should really be considered as control (V1) since they will have already received the V1 treatment by default. But the Finch dashboard will consider the user is in V2 treatment group.
For solutions, we can either:
1. Log additional metrics when we know the users received the Finch treatment. But with this, we could not use the Finch dashboard.
2. Register Synthetic field trial when we know the users received the Finch treatment. Then we could use the Finch baseboard to slice the metrics interested in.
We think that option 2 (this cl) will be helpful and convenient for us to inspect the experiment results.
Li Lin (Gerrit)
Li Lin voted Code-Review+1![]( "Open in Gerrit")
- Mark Pearson
- Tao Wu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tao Wu (Gerrit)
Tao Wu abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |