[B4B] Adapt Synthetic field trials for Bookmarks Bubble event [chromium/src : main]
Ryan Sultanem (Gerrit)
Ryan Sultanem added 1 comment![]( "Open in Gerrit")
Hey James, I have made some adaptations to the Synthetic Field trial groups based on your suggestions, thanks for pointing out the issues in the initial setup!
Please let me know what you think!
Related details
- James Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Lee (Gerrit)
James Lee added 3 comments![]( "Open in Gerrit")
Hi Ryan, thanks for this rework - I still have high-level comments/questions (I haven't done a full review).
These are around how we deal with multi-profile, and also which users should be included in the "Sync" (aka control) group.
My intuition here is that we want:
- Control (aka "Sync"): clients who have at some point completed sign in from the bookmarks promo while in the control group of the main experiment
- Treatment (aka "Butter"): clients who have at some point completed sign in from the bookmarks promo, while in the enabled group of the main experiment
I think this state should be per-client, not per profile, and I don't think it should be reset on sign out (because the client has still experienced the feature).
This is a subtle setup, and not one I have experiment with, so I think it would be good to discuss this with Lina as well as another round of review with a metrics reviewer.
constexpr char kEnableSyncFromBookmarksBubble[] =Per my high-level comment, I think that this state should be client-scope, rather than per-profile-per-account. I.e. this should be in local-state rather than profile-prefs, and should not keyed by gaia_id.
I'd also be inclined to make this a string pref with group name as value ("", "Butter", "Sync") to make it clear this is specific to the synthetic trial.
CHECK(!enabled || !base::FeatureList::IsEnabled(I think you should only be setting this pref if the user is in the experiment at all (either control or treatment groups). If the user is in the default group then you should not be setting a value (so that we don't register the field trial).
I.e. if the main experiment is at 1% control, 1% treatment, 98% not-in-experiment then we shouldn't set the pref for the 98%.
You can use [IsFeatureOverridden](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=370;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b) to check this.
I'm also not sure whether the group names should be dynamic and include a date suffix that is in sync with the main experiment setup - maybe a metrics reviewer can advise.
Related details
- Ryan Sultanem
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 2 comments![]( "Open in Gerrit")
Hi Ryan, thanks for this rework - I still have high-level comments/questions (I haven't done a full review).
These are around how we deal with multi-profile, and also which users should be included in the "Sync" (aka control) group.
My intuition here is that we want:
- Control (aka "Sync"): clients who have at some point completed sign in from the bookmarks promo while in the control group of the main experiment
- Treatment (aka "Butter"): clients who have at some point completed sign in from the bookmarks promo, while in the enabled group of the main experiment
I think this state should be per-client, not per profile, and I don't think it should be reset on sign out (because the client has still experienced the feature).
This is a subtle setup, and not one I have experiment with, so I think it would be good to discuss this with Lina as well as another round of review with a metrics reviewer.
Thank you James.
The point you are raising are interesting indeed, however my approach to limiting the groups to the currently "active" (active being the reason they are in a group, whether Control or Enabled), in this case being signed in/syncing coming in from the Bookmarks Bubble in a certain profile, comes from the nature of how they were first added to the group; which is at run time. So it felt natural to me to exclude them once they don't satisfy the inclusion to the group rule.
In a regular `start_active` Server side finch experiment those users would be in the group since start up.
My reasoning is also backed up by the fact that the groups do not actually stick, I had to add further logic to make sure that this group is set back at every session startup to make sure they are aligned.
I am also not very familiar with those Client side configurations and best practices, I just made some assumptions here based on the default configuration. Despite that, I do believe the points that you mentioned are valid!
I think it would be good to have a metrics reviewer weight in on that to better understand the Client side experiments needs!
Adding @asvi...@chromium.org who reviewed the initial CL. Your input would be really helpful here!
Hey Alexei! Please check the high level comment; we would like your input on the best way to setup these client side groups!
Thanks!
Related details
- Alexei Svitkine
- James Lee
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine added 5 comments![]( "Open in Gerrit")
- Trial purpose: Tracks users that have signed in or turned on syncNit: I think "track" isn't the right term here. I think "Tag clients" is the more accurate terminology.
have sync on - Butter off.Can you clarify if your goal is to just restrict your analysis to users who triggered the bookmarks bubble UI option to sign in and to do A/B analysis on them based on the trial?
If your trial is not starts active, then it's already the behavior except I guess here you're also triggering this on subsequent sessions. So I guess what you're after is "sticky activation", which you're doing here manually (we don't have built-in support for that, so if you need it, this is indeed the way to do it). If so, can you expand CL description (or put in a context doc) with that spelled out?
variations::SyntheticTrialAnnotationMode::kCurrentLog);Nit: Move call outside the function and just assign to the group name string_view.
constexpr char kEnableSyncFromBookmarksBubble[] =Per my high-level comment, I think that this state should be client-scope, rather than per-profile-per-account. I.e. this should be in local-state rather than profile-prefs, and should not keyed by gaia_id.
I'd also be inclined to make this a string pref with group name as value ("", "Butter", "Sync") to make it clear this is specific to the synthetic trial.
+1 to more explicitly thinking how this interacts with multiprofile and what you want the behavior to be in that case.
CHECK(!enabled || !base::FeatureList::IsEnabled(I think you should only be setting this pref if the user is in the experiment at all (either control or treatment groups). If the user is in the default group then you should not be setting a value (so that we don't register the field trial).
I.e. if the main experiment is at 1% control, 1% treatment, 98% not-in-experiment then we shouldn't set the pref for the 98%.
You can use [IsFeatureOverridden](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=370;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b) to check this.
I'm also not sure whether the group names should be dynamic and include a date suffix that is in sync with the main experiment setup - maybe a metrics reviewer can advise.
- James Lee
- Ryan Sultanem
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Lee (Gerrit)
James Lee added 1 comment![]( "Open in Gerrit")
CHECK(!enabled || !base::FeatureList::IsEnabled(Alexei SvitkineI think you should only be setting this pref if the user is in the experiment at all (either control or treatment groups). If the user is in the default group then you should not be setting a value (so that we don't register the field trial).
I.e. if the main experiment is at 1% control, 1% treatment, 98% not-in-experiment then we shouldn't set the pref for the 98%.
You can use [IsFeatureOverridden](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=370;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b) to check this.
I'm also not sure whether the group names should be dynamic and include a date suffix that is in sync with the main experiment setup - maybe a metrics reviewer can advise.
+1
Is that +1 just to not including the default group in the synthetic trial, or also to reflecting the suffix of the main experiment group names in the synthetic group names (eg. "MainExperiment_Enabled_20250526 -> SyntheticExperiment_Enabled_20250526)?
For the latter would this be via [GetFieldTrial(feature)](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=501;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b)->GetGroupNameWithoutActivation() or some other way?
Related details
- Ryan Sultanem
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine added 1 comment![]( "Open in Gerrit")
CHECK(!enabled || !base::FeatureList::IsEnabled(Alexei SvitkineI think you should only be setting this pref if the user is in the experiment at all (either control or treatment groups). If the user is in the default group then you should not be setting a value (so that we don't register the field trial).
I.e. if the main experiment is at 1% control, 1% treatment, 98% not-in-experiment then we shouldn't set the pref for the 98%.
You can use [IsFeatureOverridden](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=370;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b) to check this.
I'm also not sure whether the group names should be dynamic and include a date suffix that is in sync with the main experiment setup - maybe a metrics reviewer can advise.
James Lee+1
Is that +1 just to not including the default group in the synthetic trial, or also to reflecting the suffix of the main experiment group names in the synthetic group names (eg. "MainExperiment_Enabled_20250526 -> SyntheticExperiment_Enabled_20250526)?
For the latter would this be via [GetFieldTrial(feature)](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=501;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b)->GetGroupNameWithoutActivation() or some other way?
Reflecting the suffix sounds good to me. Your code snippet looks right (check that the returned field trial is not null though.)
Related details
- Ryan Sultanem
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 6 comments![]( "Open in Gerrit")
Thanks both for your inputs. I have updated the change to reflect your changes but I do not believe it is quite fully complete yet.
I still have some questions that I left in 2 comments where I would like your point of view / confirmation.
James: I am planning to finalize this change first before tackling the addition of the other trial (when the users see the bubble), for simplicity.
- Trial purpose: Tracks users that have signed in or turned on syncNit: I think "track" isn't the right term here. I think "Tag clients" is the more accurate terminology.
Done
variations::SyntheticTrialAnnotationMode::kCurrentLog);Nit: Move call outside the function and just assign to the group name string_view.
Done
// Remove any registered group in case the value of the feature was changed
// and is not overridden anymore.
if (!base::FeatureList::GetInstance()->IsFeatureOverridden(
switches::kSyncEnableBookmarksInTransportMode.name)) {
g_browser_process->local_state()->SetString(
kSigninFromBookmarksBubbleGroupPref, "");
return;
}This does not look very correct yet - but I was just starting something here.
I am wondering how should we actually approach this, in case the groups gets reshuffled. A way to do it would be to only register at start up if the registered group (in the pref) and the active group (from trial) match; otherwise we reset the pref.
Does that sound right?
That does seem like a complex approach though, and again I do not see any other synthetic field trial group doing that. Maybe I am missing something.
Alexei SvitkinePer my high-level comment, I think that this state should be client-scope, rather than per-profile-per-account. I.e. this should be in local-state rather than profile-prefs, and should not keyed by gaia_id.
I'd also be inclined to make this a string pref with group name as value ("", "Butter", "Sync") to make it clear this is specific to the synthetic trial.
+1 to more explicitly thinking how this interacts with multiprofile and what you want the behavior to be in that case.
Moved to a local state pref.
CHECK(!enabled || !base::FeatureList::IsEnabled(Alexei SvitkineI think you should only be setting this pref if the user is in the experiment at all (either control or treatment groups). If the user is in the default group then you should not be setting a value (so that we don't register the field trial).
I.e. if the main experiment is at 1% control, 1% treatment, 98% not-in-experiment then we shouldn't set the pref for the 98%.
You can use [IsFeatureOverridden](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=370;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b) to check this.
I'm also not sure whether the group names should be dynamic and include a date suffix that is in sync with the main experiment setup - maybe a metrics reviewer can advise.
James Lee+1
Alexei SvitkineIs that +1 just to not including the default group in the synthetic trial, or also to reflecting the suffix of the main experiment group names in the synthetic group names (eg. "MainExperiment_Enabled_20250526 -> SyntheticExperiment_Enabled_20250526)?
For the latter would this be via [GetFieldTrial(feature)](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=501;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b)->GetGroupNameWithoutActivation() or some other way?
Reflecting the suffix sounds good to me. Your code snippet looks right (check that the returned field trial is not null though.)
I am currently using `IsFeatureOverridden()` to differentiate whether the user is part of the experiment or part of the default group.
I have not yet used the `GetFieldTrial(feature)->GetGroupNameWithoutActivation()` because I am wondering how that will actually align with the definition of the groups to expose for the UMA dashboard. Will that also need to updated accordingly every time? I also see none of those groups that actually uses the active suffix method - some do update the name their groups with increment numbers though.
I would just like to confirm that before proceeding with the change.
FYI: exposing groups to UMA requires changes on the server side: (googlers-only): [synthetic_trials.py](https://source.corp.google.com/piper///depot/google3/analysis/uma/dashboards/variations/synthetic_trials.py?q=analysis%2Fuma%2Fdashboards%2Fvariations%2Fsynthetic_trials.py&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental)
Related details
- Alexei Svitkine
- James Lee
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Lee (Gerrit)
James Lee added 8 comments![]( "Open in Gerrit")
Thanks Ryan, I've left a few comment but let's discuss and please also wait for Alexei to review.
constexpr char kSigninFromBookmarksBubble[] =nit: can you add eg. a `SyntheticTrialName` or similar suffix to the constant, to clearly distinguish the pref names from the field trial names?
"UnoDesktopSigninFromBookmarksBubble";The main experiment is called `UnoDesktopBookmarksAndReadingList`. Maybe call the two synthetic trials eg.
UnoDesktopBookmarksBubbleShown
UnoDesktopBookmarksEnabledInAccountFromBubble
static constexpr char kSigninFromBookmarksBubbleButterGroup[] = "Butter";Rather than defining the group names here, I would directly read the group names of the server-side experiment and use them as the group names for the synthetic trials.
This means that you'll get a 1:1 mapping that is kept in sync as the server config changes.
If per the discussion in the comment thread you decide to stick with hard-coded names, I have a slight preference for Control/Enabled?
void ChromeSigninClient::RegisterUnoBookmarksSyntheticFieldTrial() {How about `MaybeAddUserToUnoDesktopBookmarksSyntheticFieldTrial`, to make clear that this may be a no-op, but also does more than just call the metrics API.
In a subsequent CL, you'll presumably either pass a parameter to say which trial it should be, or rename and have a similar method for the other trial?
if (!base::FeatureList::GetInstance()->IsFeatureOverridden(Small nit: I think probably `GetFieldTrial() == null` is a better check here - IIUC the difference is that `GetFieldTrial()` will only look at server-side overrides whereas `IsFeatureOverridden()` will also include client-side ones (chrome://flags and commandline args).
It doesn't make a big difference in practice, but I think it's maybe more correct to not include users who manually opt in/out of the experiment.
// Remove any registered group in case the value of the feature was changed
// and is not overridden anymore.
if (!base::FeatureList::GetInstance()->IsFeatureOverridden(
switches::kSyncEnableBookmarksInTransportMode.name)) {
g_browser_process->local_state()->SetString(
kSigninFromBookmarksBubbleGroupPref, "");
return;
}This does not look very correct yet - but I was just starting something here.
I am wondering how should we actually approach this, in case the groups gets reshuffled. A way to do it would be to only register at start up if the registered group (in the pref) and the active group (from trial) match; otherwise we reset the pref.
Does that sound right?
That does seem like a complex approach though, and again I do not see any other synthetic field trial group doing that. Maybe I am missing something.
I think this depends on:
- What approach is taken for group names (constant or server-derived) per the other comment thread
- What the product behavior is for users who leave the experiment (do they continue to receive the feature or is it turned off for them)
CHECK(!enabled || !base::FeatureList::IsEnabled(Alexei SvitkineI think you should only be setting this pref if the user is in the experiment at all (either control or treatment groups). If the user is in the default group then you should not be setting a value (so that we don't register the field trial).
I.e. if the main experiment is at 1% control, 1% treatment, 98% not-in-experiment then we shouldn't set the pref for the 98%.
You can use [IsFeatureOverridden](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=370;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b) to check this.
I'm also not sure whether the group names should be dynamic and include a date suffix that is in sync with the main experiment setup - maybe a metrics reviewer can advise.
James Lee+1
Alexei SvitkineIs that +1 just to not including the default group in the synthetic trial, or also to reflecting the suffix of the main experiment group names in the synthetic group names (eg. "MainExperiment_Enabled_20250526 -> SyntheticExperiment_Enabled_20250526)?
For the latter would this be via [GetFieldTrial(feature)](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=501;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b)->GetGroupNameWithoutActivation() or some other way?
Ryan SultanemReflecting the suffix sounds good to me. Your code snippet looks right (check that the returned field trial is not null though.)
I am currently using `IsFeatureOverridden()` to differentiate whether the user is part of the experiment or part of the default group.
I have not yet used the `GetFieldTrial(feature)->GetGroupNameWithoutActivation()` because I am wondering how that will actually align with the definition of the groups to expose for the UMA dashboard. Will that also need to updated accordingly every time? I also see none of those groups that actually uses the active suffix method - some do update the name their groups with increment numbers though.
I would just like to confirm that before proceeding with the change.
FYI: exposing groups to UMA requires changes on the server side: (googlers-only): [synthetic_trials.py](https://source.corp.google.com/piper///depot/google3/analysis/uma/dashboards/variations/synthetic_trials.py?q=analysis%2Fuma%2Fdashboards%2Fvariations%2Fsynthetic_trials.py&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental)
Ah interesting point - I guess there are two options:
1. Use hard-coded names in the client, which won't update with server config changes (eg. ramp 1%->10%, or bump min_version etc).
2. Use the actual names from the server, with an IfThisThenThat check to update synthetic_trials.py when the Finch config suffix is updated.
I guess the slight downside of 2 is that it takes a week or so for the dashboard to pick up the changes. I think that's probably OK since we need to wait for a week of data before stable analysis anyway, so that would be my preference.
@asvi...@chromium.org does that still sound good?
Related details
- Alexei Svitkine
- Ryan Sultanem
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine added 1 comment![]( "Open in Gerrit")
CHECK(!enabled || !base::FeatureList::IsEnabled(Alexei SvitkineI think you should only be setting this pref if the user is in the experiment at all (either control or treatment groups). If the user is in the default group then you should not be setting a value (so that we don't register the field trial).
I.e. if the main experiment is at 1% control, 1% treatment, 98% not-in-experiment then we shouldn't set the pref for the 98%.
You can use [IsFeatureOverridden](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=370;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b) to check this.
I'm also not sure whether the group names should be dynamic and include a date suffix that is in sync with the main experiment setup - maybe a metrics reviewer can advise.
James Lee+1
Alexei SvitkineIs that +1 just to not including the default group in the synthetic trial, or also to reflecting the suffix of the main experiment group names in the synthetic group names (eg. "MainExperiment_Enabled_20250526 -> SyntheticExperiment_Enabled_20250526)?
For the latter would this be via [GetFieldTrial(feature)](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.h;l=501;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b)->GetGroupNameWithoutActivation() or some other way?
Ryan SultanemReflecting the suffix sounds good to me. Your code snippet looks right (check that the returned field trial is not null though.)
James LeeI am currently using `IsFeatureOverridden()` to differentiate whether the user is part of the experiment or part of the default group.
I have not yet used the `GetFieldTrial(feature)->GetGroupNameWithoutActivation()` because I am wondering how that will actually align with the definition of the groups to expose for the UMA dashboard. Will that also need to updated accordingly every time? I also see none of those groups that actually uses the active suffix method - some do update the name their groups with increment numbers though.
I would just like to confirm that before proceeding with the change.
FYI: exposing groups to UMA requires changes on the server side: (googlers-only): [synthetic_trials.py](https://source.corp.google.com/piper///depot/google3/analysis/uma/dashboards/variations/synthetic_trials.py?q=analysis%2Fuma%2Fdashboards%2Fvariations%2Fsynthetic_trials.py&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental)
Ah interesting point - I guess there are two options:
1. Use hard-coded names in the client, which won't update with server config changes (eg. ramp 1%->10%, or bump min_version etc).
2. Use the actual names from the server, with an IfThisThenThat check to update synthetic_trials.py when the Finch config suffix is updated.I guess the slight downside of 2 is that it takes a week or so for the dashboard to pick up the changes. I think that's probably OK since we need to wait for a week of data before stable analysis anyway, so that would be my preference.
@asvi...@chromium.org does that still sound good?
- James Lee
- Ryan Sultanem
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 8 comments![]( "Open in Gerrit")
Ryan SultanemHi Ryan, thanks for this rework - I still have high-level comments/questions (I haven't done a full review).
These are around how we deal with multi-profile, and also which users should be included in the "Sync" (aka control) group.
My intuition here is that we want:
- Control (aka "Sync"): clients who have at some point completed sign in from the bookmarks promo while in the control group of the main experiment
- Treatment (aka "Butter"): clients who have at some point completed sign in from the bookmarks promo, while in the enabled group of the main experiment
I think this state should be per-client, not per profile, and I don't think it should be reset on sign out (because the client has still experienced the feature).
This is a subtle setup, and not one I have experiment with, so I think it would be good to discuss this with Lina as well as another round of review with a metrics reviewer.
Thank you James.
The point you are raising are interesting indeed, however my approach to limiting the groups to the currently "active" (active being the reason they are in a group, whether Control or Enabled), in this case being signed in/syncing coming in from the Bookmarks Bubble in a certain profile, comes from the nature of how they were first added to the group; which is at run time. So it felt natural to me to exclude them once they don't satisfy the inclusion to the group rule.
In a regular `start_active` Server side finch experiment those users would be in the group since start up.My reasoning is also backed up by the fact that the groups do not actually stick, I had to add further logic to make sure that this group is set back at every session startup to make sure they are aligned.
I am also not very familiar with those Client side configurations and best practices, I just made some assumptions here based on the default configuration. Despite that, I do believe the points that you mentioned are valid!
I think it would be good to have a metrics reviewer weight in on that to better understand the Client side experiments needs!
Adding @asvi...@chromium.org who reviewed the initial CL. Your input would be really helpful here!
Acknowledged
Thanks, I still have some concerns about the group name derived from the experiment name. Please check the associated comment when registering the groups after sign in/sync.
nit: can you add eg. a `SyntheticTrialName` or similar suffix to the constant, to clearly distinguish the pref names from the field trial names?
Done
The main experiment is called `UnoDesktopBookmarksAndReadingList`. Maybe call the two synthetic trials eg.
UnoDesktopBookmarksBubbleShown
UnoDesktopBookmarksEnabledInAccountFromBubble
Done
static constexpr char kSigninFromBookmarksBubbleButterGroup[] = "Butter";Rather than defining the group names here, I would directly read the group names of the server-side experiment and use them as the group names for the synthetic trials.
This means that you'll get a 1:1 mapping that is kept in sync as the server config changes.
If per the discussion in the comment thread you decide to stick with hard-coded names, I have a slight preference for Control/Enabled?
This was done temporarily until figuring out how to assign existing users to shuffling groups and exiting groups.
Changing it to use the server-side experiment group names for now until confirmed.
void ChromeSigninClient::RegisterUnoBookmarksSyntheticFieldTrial() {How about `MaybeAddUserToUnoDesktopBookmarksSyntheticFieldTrial`, to make clear that this may be a no-op, but also does more than just call the metrics API.
In a subsequent CL, you'll presumably either pass a parameter to say which trial it should be, or rename and have a similar method for the other trial?
Done
if (!base::FeatureList::GetInstance()->IsFeatureOverridden(Small nit: I think probably `GetFieldTrial() == null` is a better check here - IIUC the difference is that `GetFieldTrial()` will only look at server-side overrides whereas `IsFeatureOverridden()` will also include client-side ones (chrome://flags and commandline args).
It doesn't make a big difference in practice, but I think it's maybe more correct to not include users who manually opt in/out of the experiment.
Done
std::string_view group_name = field_trial->GetGroupNameWithoutActivation();With manual testing, this `group_name` does not seem to contain the version suffix. I was only able to read `Enabled` - I presume my session was added as the Enabled group (part of the 50% chance)
Is the missing version suffix expected for regular users or is it something specific to Development builds?
Related details
- James Lee
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 1 comment![]( "Open in Gerrit")
std::string_view group_name = field_trial->GetGroupNameWithoutActivation();With manual testing, this `group_name` does not seem to contain the version suffix. I was only able to read `Enabled` - I presume my session was added as the Enabled group (part of the 50% chance)
Is the missing version suffix expected for regular users or is it something specific to Development builds?
I actually forgot about the fieldtrial_testing_config that overrides the feature values - this is what I was logging in my manual test.
With a fake finch server and making it fetch data from the real canary/beta branches, I was able to read the proper values.
Thanks James for your inputs (offline) on this.
Related details
- James Lee
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 2 comments![]( "Open in Gerrit")
Left a last comment, and then I think I can proceed with implementing the other synthetic group. I believe I will keep it within this CL to have a single merge back change.
std::string_view group_name = g_browser_process->local_state()->GetString(With this setup, imagine the following sequence happens:
- User is signed out.
- Signs in through the bubble.
- The synthetic trial group assigned is: `Enabled_Date1` - assuming they are part of the enabled group.
- Closes Chrome.
- Some server side changes happens to the main experiment reshuffling the groups.
- User opens Chrome again.
At this point they could in theory be in either:
- Enabled_Date2
- Control_Date2
- Default
The current code will keep logging them for `Enabled_Date1`. And will only reset their synthetic trial group if the experiment was stopped.
Is this acceptable? Or should we migrate them to the proper group.
Related details
- James Lee
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Lee (Gerrit)
James Lee added 3 comments![]( "Open in Gerrit")
// Synthetic field trial for users that have signed in through the Bookmarksnit: replace "signed in" with "enabled account bookmarks" or similar in the comment?
ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(Optional nit: although it's only one line, I have a slight preference for extracting this to a separate `RegisterSyntheticTrialsFromPrefs()` or similar that is called from both here and `DoFinalInit()`.
It makes it more obvious from reading that the state is entirely mastered in the pref (and avoids copy-paste bugs once you have two trials).
std::string_view group_name = g_browser_process->local_state()->GetString(With this setup, imagine the following sequence happens:
- User is signed out.
- Signs in through the bubble.
- The synthetic trial group assigned is: `Enabled_Date1` - assuming they are part of the enabled group.
- Closes Chrome.
- Some server side changes happens to the main experiment reshuffling the groups.
- User opens Chrome again.
At this point they could in theory be in either:
- Enabled_Date2
- Control_Date2
- Default
The current code will keep logging them for `Enabled_Date1`. And will only reset their synthetic trial group if the experiment was stopped.
Is this acceptable? Or should we migrate them to the proper group.
As discussed offline, I think that the most reasonable behavior here is to treat the synthetic trial assignment is permanently sticky once set - it therefore mirrors the state at the point the user first interacted with the feature.
Therefore you don't need to check `base::FeatureList::GetInstance()->IsFeatureOverridden()` above.
Related details
- Ryan Sultanem
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 4 comments![]( "Open in Gerrit")
// Synthetic field trial for users that have signed in through the Bookmarksnit: replace "signed in" with "enabled account bookmarks" or similar in the comment?
Done
ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(Optional nit: although it's only one line, I have a slight preference for extracting this to a separate `RegisterSyntheticTrialsFromPrefs()` or similar that is called from both here and `DoFinalInit()`.
It makes it more obvious from reading that the state is entirely mastered in the pref (and avoids copy-paste bugs once you have two trials).
Done
std::string_view group_name = g_browser_process->local_state()->GetString(James LeeWith this setup, imagine the following sequence happens:
- User is signed out.
- Signs in through the bubble.
- The synthetic trial group assigned is: `Enabled_Date1` - assuming they are part of the enabled group.
- Closes Chrome.
- Some server side changes happens to the main experiment reshuffling the groups.
- User opens Chrome again.
At this point they could in theory be in either:
- Enabled_Date2
- Control_Date2
- Default
The current code will keep logging them for `Enabled_Date1`. And will only reset their synthetic trial group if the experiment was stopped.
Is this acceptable? Or should we migrate them to the proper group.
As discussed offline, I think that the most reasonable behavior here is to treat the synthetic trial assignment is permanently sticky once set - it therefore mirrors the state at the point the user first interacted with the feature.
Therefore you don't need to check `base::FeatureList::GetInstance()->IsFeatureOverridden()` above.
Acknowledged
// Remove any registered group in case the value of the feature was changed
// and is not overridden anymore.
if (!base::FeatureList::GetInstance()->IsFeatureOverridden(
switches::kSyncEnableBookmarksInTransportMode.name)) {
g_browser_process->local_state()->SetString(
kSigninFromBookmarksBubbleGroupPref, "");
return;
}James LeeThis does not look very correct yet - but I was just starting something here.
I am wondering how should we actually approach this, in case the groups gets reshuffled. A way to do it would be to only register at start up if the registered group (in the pref) and the active group (from trial) match; otherwise we reset the pref.
Does that sound right?
That does seem like a complex approach though, and again I do not see any other synthetic field trial group doing that. Maybe I am missing something.
I think this depends on:
- What approach is taken for group names (constant or server-derived) per the other comment thread
- What the product behavior is for users who leave the experiment (do they continue to receive the feature or is it turned off for them)
This was discussed offline - we also decided that users groups will stick regardless of any change after being set.
Related details
- James Lee
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 2 comments![]( "Open in Gerrit")
Thanks Alexei for your inputs! I will move you to CC.
Adding David, for `chrome/browser/prefs/browser_prefs.cc` owner.
Thanks!
Can you clarify if your goal is to just restrict your analysis to users who triggered the bookmarks bubble UI option to sign in and to do A/B analysis on them based on the trial?
If your trial is not starts active, then it's already the behavior except I guess here you're also triggering this on subsequent sessions. So I guess what you're after is "sticky activation", which you're doing here manually (we don't have built-in support for that, so if you need it, this is indeed the way to do it). If so, can you expand CL description (or put in a context doc) with that spelled out?
Enriched the cl description with more details to reflect that. Thanks!
Related details
- David Roger
- James Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Lee (Gerrit)
James Lee voted and added 1 comment![]( "Open in Gerrit")
Votes added by James Lee
| Code-Review | +1 |
1 comment
- David Roger
- Ryan Sultanem
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Roger (Gerrit)
David Roger voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Roger
| Code-Review | +1 |
1 comment
Related details
- Ryan Sultanem
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ryan Sultanem
| Commit-Queue | +1 |
2 comments
Hey David and James, could you please check the added changes?
I preferred to add the second synthetic field trial in the same change to simplify the merge request. PTAL - sorry about having to recheck!
Thanks!
ChromeSigninClient::
MaybeAddUserToBookmarksBubblePromoShownSyntheticFieldTrial();This currently does not filter out the users that are signed in (not syncing) for the control group of the main experiment, for which the Sync promo will be shown.
I am not sure what was the latest decision on that detail was, if needed we can filter them out easily here.
Related details
- David Roger
- James Lee
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Roger (Gerrit)
David Roger voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Roger
| Code-Review | +1 |
2 comments
this looks good superficially, but I am not an expert with synthetic field trials at all.
ChromeSigninClient::
MaybeAddUserToBookmarksBubblePromoShownSyntheticFieldTrial();This currently does not filter out the users that are signed in (not syncing) for the control group of the main experiment, for which the Sync promo will be shown.
I am not sure what was the latest decision on that detail was, if needed we can filter them out easily here.
It probably does not matter much? Are we going to use the control group?
Related details
- James Lee
- Ryan Sultanem
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Lee (Gerrit)
James Lee voted and added 1 comment![]( "Open in Gerrit")
Votes added by James Lee
| Code-Review | +1 |
1 comment
ChromeSigninClient::
MaybeAddUserToBookmarksBubblePromoShownSyntheticFieldTrial();David RogerThis currently does not filter out the users that are signed in (not syncing) for the control group of the main experiment, for which the Sync promo will be shown.
I am not sure what was the latest decision on that detail was, if needed we can filter them out easily here.
It probably does not matter much? Are we going to use the control group?
There is some more context on this linked in the bug (with still some open discussion).
I think we should land this CL as-is, and if we decide to slightly tweak the groups for this synthetic trial then that could be done in a subsequent CL.
Related details
- Ryan Sultanem
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Lee (Gerrit)
James Lee added 1 comment![]( "Open in Gerrit")
MaybeAddUserToUnoBookmarksSyntheticFieldTrial(The code coverage tool is reporting no coverage for this method at all - is that really true, or is that a consequence of some other failures in the run?
I think that there should at least be code coverage to ensure no crashes (even if we don't fully test that the group assignments are as expected).
Related details
- Ryan Sultanem
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 3 comments![]( "Open in Gerrit")
Thanks! Since I modified a new file (added a new test), your +1s were removed.
Could you please check again? Sorry about that!
(Maybe one of you would be enough for approval)
The code coverage tool is reporting no coverage for this method at all - is that really true, or is that a consequence of some other failures in the run?
I think that there should at least be code coverage to ensure no crashes (even if we don't fully test that the group assignments are as expected).
Added a browser test to simulate the addition of the group in the prefs.
Enabling the field trial for the disabled/control groups does not seem to be part of the infra. Only testing for the enabled group - which should be enough.
ChromeSigninClient::
MaybeAddUserToBookmarksBubblePromoShownSyntheticFieldTrial();David RogerThis currently does not filter out the users that are signed in (not syncing) for the control group of the main experiment, for which the Sync promo will be shown.
I am not sure what was the latest decision on that detail was, if needed we can filter them out easily here.
James LeeIt probably does not matter much? Are we going to use the control group?
There is some more context on this linked in the bug (with still some open discussion).
I think we should land this CL as-is, and if we decide to slightly tweak the groups for this synthetic trial then that could be done in a subsequent CL.
- David Roger
- James Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Lee (Gerrit)
James Lee voted Code-Review+1![]( "Open in Gerrit")
- David Roger
- Ryan Sultanem
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[B4B] Adapt Synthetic field trials for Bookmarks Bubble event
This CL is a followup of http://crrev.com/c/6542647 that repurposes
the created Synthetic field trial:
`kBookmarksBubblePromoShownSyntheticTrialName`
- Trial purpose: Tag clients that have signed in or turned on sync
from the Bookmarks Bubble.
- Butter Group (Enabled): With the feature enabled, those users will
have butter on.
- Sync Group (Disabled): With the feature disabled, those users will
have sync on - Butter off.
Also adds a second Synthetic field trial for users that have been
shown the sign in or sync promo: `UnoDesktopBookmarksBubblePromoShown`.
The group names associated with Synthetic field trial are extracted
from the main experiment group names, and will adapt based on any
change done to the main experiment without client changes.
The groups are sticky and will not be modified; they are reset on
startup based on the value that was set after sign-in/sync. Even if
the group names of the main experiment are modified, it will not be
reflected in users that have groups already assigned.
- M chrome/browser/prefs/browser_prefs.cc
- M chrome/browser/signin/chrome_signin_client.cc
- M chrome/browser/signin/chrome_signin_client.h
- M chrome/browser/signin/chrome_signin_client_browsertest.cc
- M chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
Code-Review: +1 by James Lee
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |