Record default search engine changes #10300
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@bsclifton Here's a draft PR based on your sketch. I'd appreciate it if you could look it over and comment. Question: I don't report the "did not switch" options. Would it work to call some alternate report function from the |
iefremov
reviewed
Sep 29, 2021
iefremov
reviewed
Sep 29, 2021
iefremov
reviewed
Sep 29, 2021
iefremov
reviewed
Sep 29, 2021
bsclifton
approved these changes
Sep 30, 2021
There was a problem hiding this comment.
Bucket issue aside, LGTM 👍
Would be awesome to re-enable the related tests and add a test for this! 😄 I'd be happy to re-review
rillian
force-pushed
the
p3a-search-switch
branch
2 times, most recently
from
e6d0644 to
c3c3300
Compare
|
@goodov How does this look? |
goodov
reviewed
Oct 5, 2021
rillian
force-pushed
the
p3a-search-switch
branch
from
c3c3300 to
d2e90e0
Compare
|
@goodov What do you think about this approach? Any suggestions on how to access the PrefsRegistry from here? |
Add a P3A probe to record when the default search engine is changed, and whether it's a transition between Brave Search or one of the other major services. This helps understand what options are seem better to users. Keep track of the previous default search engine url. The event the tracker observes only indicates something has changed, and by then the previous state is no longer accessible; we need this to correctly mark the transition in our matrix, separate from whatever the original default was at startup. Move the actual record step into an object method so it can access the saved previous default. This simplifies calling it at startup to set the initial kNoSwitch answer. Note: this means we only report a change if the ping is sent from the same application instance. We don't reset to kNoSwitch at the end of the weekly sample cadence, and restarting the browser always resets. Closes brave/brave-browser#18224
Add a browser test to verify we record transitions as expected. This is disabled for unstable results, similar to SearchEngineProviderP3ATest.DefaultSearchEngineP3A.
This is a helper class to record events for later P3A reporting. It remembers a sequence of enum values for a week, and can return the most recently-added value within that time period, if any.
Use WeeklyEventStorage to keep track of default search engine changes over the previous week, so we can return an accurate report of whether a switch happened during the P3A measurement period. This registers a new pref to back the WeeklyEventStorage data.
rillian
force-pushed
the
p3a-search-switch
branch
from
d2e90e0 to
ca79451
Compare
|
Ready for review again! I disabled the browser test for the |
goodov
reviewed
Oct 14, 2021
goodov
reviewed
Oct 14, 2021
goodov
reviewed
Oct 14, 2021
goodov
reviewed
Oct 14, 2021
goodov
reviewed
Oct 14, 2021
goodov
reviewed
Oct 14, 2021
Specialize WeeklyEventStorage to use the basic `int` type for event codes, rather than being a template class. The removes type information at call sites using an `enum class` but also reduces code bloat, which we prefer.
Like the other SearchEngineTracker browser test, this gives unstable results in ci, tracked in brave/brave-browser#13057. Disable the test for now, and remove the `MergeHistogramDeltasForTesting` which we don't expect is necessary and made no difference in testing.
|
I've not been able to get the test stable, so I've disabled it again. Given the priority, I think we should land without browsertest coverage for the P3A probe. |
|
I think the android ci failure in unrelated. Restarted the job.
|
|
Likewise the other groovy failures, confirmed by Mihai's post on slack. Looks like these will all fail notifying success. I'll restart them. |
goodov
reviewed
Oct 19, 2021
goodov
force-pushed
the
p3a-search-switch
branch
from
b1f5d61 to
49b17a1
Compare
goodov
approved these changes
Oct 19, 2021
|
Thanks for the fixes! @iefremov ok to merge? |
iefremov
reviewed
Oct 19, 2021
iefremov
reviewed
Oct 19, 2021
iefremov
reviewed
Oct 19, 2021
iefremov
reviewed
Oct 19, 2021
iefremov
reviewed
Oct 19, 2021
iefremov
approved these changes
Oct 19, 2021
- Document the motivation for setting the region. - Fix a comment typo.
This was referenced Oct 20, 2021
|
Verification passed on
Brave default SE supported region - USExisting user Default was not Brave, did not switch - PASSED
Default was Brave, did not switch - PASSED
Default was Brave, switched to Google - PASSED
Default was Brave, switched to DDG - PASSED
Default was Brave, switched to Other (NOT Google OR DDG) - PASSED
Default was Google, switched to Brave - PASSED
Default was DDG, switched to Brave - PASSED
Default was Other (NOT Google OR DDG), switched to Brave - PASSED
Default was Other (not Brave), switched to Other (not Brave) - PASSED
New user Clean profile, default SE is brave - PASSED
Clean profile, default is Brave, switch to Google - PASSED
Clean profile, default is Brave, switch to Google and again switch back to Brave- PASSED
Clean profile, default is Brave, switch to DDG - PASSED
Clean profile, default is Brave, switched to Other (NOT Google OR DDG) - PASSED
Clean profile, default is Brave, switch to Other (NOT Google OR DDG), switched to Brave- PASSED
Clean profile, default to Other (not Brave), switched to Other (not Brave)- PASSED
Brave default SE not supported region - INNew user Clean profile, default SE is Google - PASSED
Clean profile, default is Google, switch to Brave - PASSED
Clean profile, default is Google, switch to DDG - PASSED
Clean profile, default is Google, switch to Brave and then switch to Bing - PASSED
Existing user Default was not Brave, did not switch - PASSED
Default was Brave, did not switch - PASSED
Default was Brave, switched to Google - PASSED
Default was Brave, switched to DDG - PASSED
Default was Other (not Brave), switched to Other (not Brave) - PASSED
Default was Other (NOT Google OR DDG), switched to Brave - PASSED
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a P3A probe to record when the default search engine is changed,
and whether it's a transition between Brave Search or one of the
other major services. This helps understand what options are seem
better to users.
Closes brave/brave-browser#18224
Resolves
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
brave://local-state.Brave.Search.SwitchEngineentry under p3a.logs. (ctrl/cmd-f to search is helpful)0:local-statetabBrave.Search.SwitchEnginehas changed accordingly. (ctrl/cmd-g to repeat the last search is helpful)The measurement looks at both the current and the previous search engine, so it's important to check both switching to and away from Brave Search. However, if the list has 7 possible default search engines, that's 49 combinations, with variations by region, so exhaustive testing is too onerous. Switching to and from Brave Search a couple of times, and once between, say, Google and DuckDuckGo is sufficient.
Brave.SearchSwitchEnginevalues represent the following transition options: