Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add sponsored image P3A instrumentation for New Tab Page #6924

Merged
merged 9 commits into from
Nov 4, 2020
Merged

Add sponsored image P3A instrumentation for New Tab Page #6924

merged 9 commits into from
Nov 4, 2020

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Oct 21, 2020 •
edited
Loading

Fixes brave/brave-browser#12267

Submitter Checklist:

Test Plan:

See brave/brave-browser#12267

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

@bsclifton bsclifton self-assigned this Oct 21, 2020
@bsclifton bsclifton changed the title WIP :Bsc ntp p3a WIP: Add Sponsored image P3A instrumentation for New Tab Page Oct 21, 2020
@bsclifton bsclifton changed the title WIP: Add Sponsored image P3A instrumentation for New Tab Page WIP: Add sponsored image P3A instrumentation for New Tab Page Oct 22, 2020
@bsclifton bsclifton marked this pull request as ready for review October 22, 2020 06:07
@bsclifton bsclifton changed the title WIP: Add sponsored image P3A instrumentation for New Tab Page Add sponsored image P3A instrumentation for New Tab Page Oct 22, 2020
@bsclifton bsclifton added this to the 1.18.x - Nightly milestone Oct 22, 2020
@bsclifton
Copy link
Member Author

bsclifton commented Oct 22, 2020 •
edited
Loading

Fixing GN deps that I missed; should have that pushed soon

edit: fixed! 🎉

@bsclifton bsclifton requested a review from aekeus October 22, 2020 07:41
iefremov
iefremov reviewed Oct 23, 2020
View reviewed changes
components/weekly_storage/weekly_storage.cc
clock_->Now() - base::TimeDelta::FromDays(kDaysInWeek);
std::list<DailyValue> last_weeks_daily_values(daily_values_.size());
auto copied_it =
std::copy_if(daily_values_.begin(), daily_values_.end(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we can do it in one pass without making a new vector? Just by finding maximum among values for which i.day > n_days_ago

Copy link
Member Author

@bsclifton bsclifton Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since std::max_element is strictly comparing left vs right (and doing less than)... what should be returned if the date range is invalid?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably end()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left as-is for now, but I can write a test program and figure it out if you'd like (I'm just not confident in doing it in one go without writing a test program)

@bsclifton bsclifton force-pushed the bsc-ntp-p3a branch 2 times, most recently from 132015a to f7ec870 Compare October 27, 2020 05:31
@bsclifton bsclifton added the CI/skip Do not run CI builds (except noplatform) label Oct 27, 2020
@bsclifton bsclifton force-pushed the bsc-ntp-p3a branch 2 times, most recently from 83e6770 to e5b3126 Compare October 27, 2020 08:28
@iefremov
Copy link
Contributor

iefremov commented Nov 2, 2020

btw, if we have any browser tests for NTP it should be quite easy to add quick checks for proper histograms using base::HistogramTester

@bsclifton bsclifton force-pushed the bsc-ntp-p3a branch 2 times, most recently from 388450b to ad062c0 Compare November 3, 2020 05:36
@bsclifton
Copy link
Member Author

Rebased so CI can get a nice clean run 😄

@bsclifton bsclifton force-pushed the bsc-ntp-p3a branch 3 times, most recently from 376307a to 138e0d4 Compare November 4, 2020 06:29
@bsclifton
Copy link
Member Author

Windows has one unrelated browser-test failure(see brave/brave-browser#12295)

13:58:33  1 test failed:
13:58:33      RewardsContributionBrowserTest.RecurringTipForUnverifiedPublisher (../../brave/components/brave_rewards/browser/test/rewards_contribution_browsertest.cc:327)

macOS has three unrelated browser test failures:

14:06:58  3 tests failed:
14:06:58      RewardsContributionBrowserTest.PanelMonthlyTipActions (../../brave/components/brave_rewards/browser/test/rewards_contribution_browsertest.cc:954)
14:06:58      RewardsContributionBrowserTest.PanelMonthlyTipAmount (../../brave/components/brave_rewards/browser/test/rewards_contribution_browsertest.cc:927)
14:06:58      RewardsContributionBrowserTest.TipNonIntegralAmount (../../brave/components/brave_rewards/browser/test/rewards_contribution_browsertest.cc:603)

Going to go ahead and merge here 😄👍

@bsclifton bsclifton merged commit 54603a0 into master Nov 4, 2020
@bsclifton bsclifton deleted the bsc-ntp-p3a branch November 4, 2020 22:50
@bsclifton
Copy link
Member Author

Updated questions! https://github.com/brave/brave-browser/wiki/P3A 👍

@bsclifton bsclifton mentioned this pull request Nov 5, 2020
Merged
4 tasks
bsclifton added a commit that referenced this pull request Nov 5, 2020
@bsclifton
Merge pull request #6924 from brave/bsc-ntp-p3a
9ab6f77 
Add sponsored image P3A instrumentation for New Tab Page
@kjozwiak
Copy link
Member

Verification PASSED on macOS 10.15.7 x64 using the following build:

1.18.44 Chromium: 87.0.4280.49 (Official Build) unknown (x86_64)
--
Revision | f77f85899646b42a1d3c8ff36794e00becab9171-refs/branch-heads/4280@{#1115}
OS | macOS Version 10.15.7 (Build 19H2)

Brave.NTP.SponsoredImagesEnabled

Ensured that Brave.NTP.SponsoredImagesEnabled appeared as 1 under brave://local-state on a new install:

"Brave.NTP.SponsoredImagesEnabled": {
 "sent": false,
 "value": "1"

Ensured that Brave.NTP.SponsoredImagesEnabled appeared as 0 under brave://local-state once sponsored images were disabled via brave://settings/newTab:

"Brave.NTP.SponsoredImagesEnabled": {
 "sent": false,
 "value": "0"

Ensured that Brave.NTP.SponsoredImagesEnabled appeared as 1 under brave://local-state once sponsored images were enabled via Customize under brave://newtab:

 "Brave.NTP.SponsoredImagesEnabled": {
  "sent": false,
  "value": "1"

Brave.NTP.CustomizeUsageStatus

Ensured that Brave.NTP.CustomizeUsageStatus appeared as 0 under brave://local-state on a new profile:

"Brave.NTP.CustomizeUsageStatus": {
 "sent": false,
 "value": "0"

Ensured that Brave.NTP.CustomizeUsageStatus appeared as 1 under brave://local-state once Customize has been clicked via brave://newtab

"Brave.NTP.CustomizeUsageStatus": {
 "sent": false,
 "value": "1"

Ensured that Brave.NTP.CustomizeUsageStatus appeared as 2 under brave://local-state once background images was disabled via Customize under brave://newtab

 "Brave.NTP.CustomizeUsageStatus": {
  "sent": false,
  "value": "2"

Brave.NTP.NewTabsCreated and Brave.NTP.SponsoredNewTabsCreated

Moved the macOS date two days behind, launched Brave and opened four tabs:

"customize_p3a_usage": 2,
  "get_initial_sr_component_in_progress": true,
    "p3a_new_tabs_created": [ {
      "day": 1604638800.0,
      "value": 4.0
      } ],

"Brave.NTP.NewTabsCreated": {
 "sent": false,
 "value": "2"

Closed Brave and moved the date forward by a single day, launched Brave and opened 9 tabs:

"p3a_new_tabs_created": [ {
 	"day": 1604898000.0,
 	"value": 9.0
 }, {
	 "day": 1604811600.0,
	 "value": 4.0
 } ],

"Brave.NTP.NewTabsCreated": {
 "sent": false,
 "value": "3"

Closed Brave and set the date to today, launched Brave and opened 2 tabs:

"p3a_new_tabs_created": [ {
  "day": 1604984400.0,
    "value": 4.0
   }, {
   day": 1604898000.0,
   "value": 9.0
   }, {
    "day": 1604811600.0,
    "value": 3.0
} ],

"Brave.NTP.NewTabsCreated": {
  "sent": false,
  "value": "3"

Ensured that p3a_sponsored_new_tabs_created is displaying the correct amount of sponsored images that has been viewed:

"p3a_sponsored_new_tabs_created": [ {
  "day": 1604898000.0,
  "value": 2.0
  }, {
  "day": 1604811600.0,
  "value": 1.0
} ]

Ensured that the p3a_sponsored_new_tabs_created displayed the correct answer:

  • highest # of sponsored images (2) / highest # of tabs (9) * 100 = 22.22%
"Brave.NTP.SponsoredNewTabsCreated": {
 "sent": false,
 "value": "3"

bbondy pushed a commit that referenced this pull request Nov 10, 2020
@bsclifton @bbondy
Merge pull request #6924 from brave/bsc-ntp-p3a
86c799d 
Add sponsored image P3A instrumentation for New Tab Page
@kjozwiak kjozwiak mentioned this pull request Nov 13, 2020
Closed
bridiver
bridiver reviewed Jul 9, 2024
View reviewed changes
browser/profiles/profile_util.h
@@ -49,6 +49,16 @@ bool IsRegularProfile(content::BrowserContext* profile);

bool IsTorDisabledForProfile(Profile* profile);

// Specifically used to record if sponsored images are enabled.
// Called from BraveAppearanceHandler and BraveNewTabMessageHandler
void RecordSponsoredImagesEnabledP3A(Profile* profile);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this stuff doesn't belong here, adding a bunch of additional deps to this target is exactly the opposite of what we want here because it already has check_includes = false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver left review comments

@petemill petemill petemill left review comments

@iefremov iefremov iefremov approved these changes

@simonhong simonhong simonhong approved these changes

@aekeus aekeus Awaiting requested review from aekeus

Assignees

@bsclifton bsclifton

Labels
None yet
Projects
None yet
Milestone
1.18.x - Release
Development

Successfully merging this pull request may close these issues.

P3A for New Tab Page
6 participants
@bsclifton @iefremov @kjozwiak @bridiver @petemill @simonhong