Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1977423002:
Added UMA stats to track the budget for any service worker which receives a (Closed)
|
Created:
4 years, 7 months ago by harkness Modified:
4 years, 7 months ago CC:
chromium-reviews, Peter Beverloo, mvanouwerkerk+watch_chromium.org, johnme+watch_chromium.org, harkness+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded UMA stats to track the budget for any service worker which receives a
push notification and the SES score for service workers which have a low/no
budget at the time of processing a push notification.
This will be used to tune the heuristic for allowing service workers to do
background processing without notifying the user every time.
BUG=602244
Committed: https://crrev.com/3f1dda7398a661f1c4e7834674bd450500007ff9
Cr-Commit-Position: refs/heads/master@{#394903}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Integrated code review comments #
Total comments: 7
Patch Set 3 : Updated UMA macro and removed array of costs. #
Total comments: 6
Patch Set 4 : Added NOTREACHED and nits #
Total comments: 6
Patch Set 5 : Updated histogram description #Patch Set 6 : Post-rebase include fix #
Messages
Total messages: 34 (13 generated)
harkness@chromium.org changed reviewers: + peter@chromium.org
This review both increases the base cost of processing background work from 1 to 2 and also adds UMAs to track budget/SES for service workers which are processing push messages.
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977423002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977423002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:35: const double kBackgroundProcessingCost = 2.0; What's your take on explicitly calling out that this is the budget for notifications, rather than "background processing" in general? (Similar to SiteEngagementScore::Get*Points().) https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:94: UMA_HISTOGRAM_COUNTS("PushMessaging.SESForLowBudgetOrigin", ses); Can we just inline these calls? No need for them to have their own methods, since they're only used once. https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service_unittest.cc:187: double newBudget = nit: new_budget https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_browsertest.cc:830: EXPECT_EQ(buckets[0].min, 0); nit: EXPECT_EQ(expected, actual) Elsewhere and with "assert" as well. https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_browsertest.cc:839: std::vector<base::Bucket> sesForNoBudgetBuckets = nit: Chromium style guide wants us to name things like: site_engagement_score_for_no_budget_buckets (no acronyms, hacker_style) Which leads me to my next suggestion: let's figure out a shorter name? What about just "samples" or "buckets" :) https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/push_messaging_notification_manager.cc (right): https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_notification_manager.cc:60: void RecordBackgroundBudget(double budget) { dito re: inline https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_notification_manager.cc:265: double backgroundProcessingCost = nit: background_processing_cost https://codereview.chromium.org/1977423002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1977423002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42375: + <owner>harkness@google.com</owner> nit: @chromium.org for each of these. Not sure why our other ones aren't.
https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:35: const double kBackgroundProcessingCost = 2.0; On 2016/05/17 11:04:34, Peter Beverloo wrote: > What's your take on explicitly calling out that this is the budget for > notifications, rather than "background processing" in general? (Similar to > SiteEngagementScore::Get*Points().) I've split it out now, although it makes the SES recording for no/low budget a bit weird if there could be different costs. However, that will be an issue we'll have to deal with when we go with the tri state anyway. https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:94: UMA_HISTOGRAM_COUNTS("PushMessaging.SESForLowBudgetOrigin", ses); On 2016/05/17 11:04:34, Peter Beverloo wrote: > Can we just inline these calls? No need for them to have their own methods, > since they're only used once. Done. https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service_unittest.cc:187: double newBudget = On 2016/05/17 11:04:34, Peter Beverloo wrote: > nit: new_budget actually, refactored a bit so renamed the variable. https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_browsertest.cc:830: EXPECT_EQ(buckets[0].min, 0); On 2016/05/17 11:04:34, Peter Beverloo wrote: > nit: EXPECT_EQ(expected, actual) > > Elsewhere and with "assert" as well. Done. https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_browsertest.cc:839: std::vector<base::Bucket> sesForNoBudgetBuckets = On 2016/05/17 11:04:34, Peter Beverloo wrote: > nit: Chromium style guide wants us to name things like: > > site_engagement_score_for_no_budget_buckets (no acronyms, hacker_style) > > Which leads me to my next suggestion: let's figure out a shorter name? What > about just "samples" or "buckets" :) Somewhat shortened, but I still kept some context. I don't mind the extra length in tests to add readability. https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/push_messaging_notification_manager.cc (right): https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_notification_manager.cc:60: void RecordBackgroundBudget(double budget) { On 2016/05/17 11:04:34, Peter Beverloo wrote: > dito re: inline Done. https://codereview.chromium.org/1977423002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_notification_manager.cc:265: double backgroundProcessingCost = On 2016/05/17 11:04:34, Peter Beverloo wrote: > nit: background_processing_cost Done. https://codereview.chromium.org/1977423002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1977423002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42375: + <owner>harkness@google.com</owner> On 2016/05/17 11:04:34, Peter Beverloo wrote: > nit: @chromium.org for each of these. Not sure why our other ones aren't. Done.
https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:157: UMA_HISTOGRAM_COUNTS("PushMessaging.SESForLowBudgetOrigin", ses_score); To this and the other histograms: UMA_HISTOGRAM_COUNTS counts 50 buckets in a range of 1 to 1,000,000. We know |budget| to be in range of [0, 100], so please use UMA_HISTOGRAM_COUNTS_100 instead. That macro counts 50 buckets in a range of 1 to a hundred. Since buckets have higher granularity for the lower counts, I also wonder what we gain by having three separate histograms. (I know we discussed this in the past, but I forgot— sorry!) We know that- (1) PushMessaging.BackgroundBudget will record budgets in the range of [0, 100]. (2) PushMessaging.SESForNoBudgetOrigin will record budgets in the range of [0, 1.99]. (3) PushMessaging.SESForLowBudgetOrigin will record budgets in the range of [2, 3.99]. In addition, lines 151-157 effectively are a layering violation, because it makes the assumption that budget is being received for push messaging. Would it make sense to only keep (1)? Michael knows the mechanics of UMA better than I do. https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:36: enum CostType { nit: enum class https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:65: static double costs_[]; This can live in an anonymous namespace, no need to expose it in the interface. In general, it would be cleaner to just have an switch() case in GetBackgroundProcessingCost, that can immediately give us compile-time verification that we didn't accidentally forget about adding a value when introducing a new cost, if you omit the default case.
https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:157: UMA_HISTOGRAM_COUNTS("PushMessaging.SESForLowBudgetOrigin", ses_score); On 2016/05/17 13:51:01, Peter Beverloo wrote: > Since buckets have higher granularity for the lower counts, I also wonder what > we gain by having three separate histograms. (I know we discussed this in the > past, but I forgot— sorry!) > > We know that- > (1) PushMessaging.BackgroundBudget will record budgets in the range of [0, > 100]. > (2) PushMessaging.SESForNoBudgetOrigin will record budgets in the range of [0, > 1.99]. > (3) PushMessaging.SESForLowBudgetOrigin will record budgets in the range of > [2, 3.99]. Scratch this part, sorry! You're recording the *SES*, not the budget, I need to read better :).
https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:157: UMA_HISTOGRAM_COUNTS("PushMessaging.SESForLowBudgetOrigin", ses_score); On 2016/05/17 13:51:01, Peter Beverloo wrote: > To this and the other histograms: > > UMA_HISTOGRAM_COUNTS counts 50 buckets in a range of 1 to 1,000,000. We know > |budget| to be in range of [0, 100], so please use UMA_HISTOGRAM_COUNTS_100 > instead. That macro counts 50 buckets in a range of 1 to a hundred. > > Since buckets have higher granularity for the lower counts, I also wonder what > we gain by having three separate histograms. (I know we discussed this in the > past, but I forgot— sorry!) > > We know that- > (1) PushMessaging.BackgroundBudget will record budgets in the range of [0, > 100]. > (2) PushMessaging.SESForNoBudgetOrigin will record budgets in the range of [0, > 1.99]. > (3) PushMessaging.SESForLowBudgetOrigin will record budgets in the range of > [2, 3.99]. > > In addition, lines 151-157 effectively are a layering violation, because it > makes the assumption that budget is being received for push messaging. > > Would it make sense to only keep (1)? Michael knows the mechanics of UMA better > than I do. I updated to UMA_HISTOGRAM_COUNTS_100 for all three and moved the SES related ones into push_messaging_notification_manager.cc https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:36: enum CostType { On 2016/05/17 13:51:02, Peter Beverloo wrote: > nit: enum class Done. https://codereview.chromium.org/1977423002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:65: static double costs_[]; On 2016/05/17 13:51:01, Peter Beverloo wrote: > This can live in an anonymous namespace, no need to expose it in the interface. > > In general, it would be cleaner to just have an switch() case in > GetBackgroundProcessingCost, that can immediately give us compile-time > verification that we didn't accidentally forget about adding a value when > introducing a new cost, if you omit the default case. As discussed in person, I'm just returning the result values from GetCost, rather than defining a constant value and then returning that.
lgtm, thanks! https://codereview.chromium.org/1977423002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1977423002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:105: // No default case. nit: -2 indent https://codereview.chromium.org/1977423002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:107: DCHECK(false); Since DCHECKS() are removed from release builds, this will actually cause a compile failure because there is no return value. What about: NOTREACHED(); return SiteEngagementScore::GetMaxPoints(); https://codereview.chromium.org/1977423002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1977423002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:63: static double costs_[]; nit: remove
harkness@chromium.org changed reviewers: + alexis@janeasystems.com
Alexi, could you please take a look at tools/metrics/histograms/histograms.xml changes? Thanks, Jen https://codereview.chromium.org/1977423002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1977423002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:105: // No default case. On 2016/05/17 15:54:28, Peter Beverloo wrote: > nit: -2 indent Done. https://codereview.chromium.org/1977423002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:107: DCHECK(false); On 2016/05/17 15:54:28, Peter Beverloo wrote: > Since DCHECKS() are removed from release builds, this will actually cause a > compile failure because there is no return value. What about: > > NOTREACHED(); > return SiteEngagementScore::GetMaxPoints(); Done. https://codereview.chromium.org/1977423002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1977423002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:63: static double costs_[]; On 2016/05/17 15:54:28, Peter Beverloo wrote: > nit: remove Done.
harkness@chromium.org changed reviewers: + asvitkine@chromium.org - alexis@janeasystems.com
Alexi, could you please take a look at tools/metrics/histograms/histograms.xml changes? Thanks, Jen
https://codereview.chromium.org/1977423002/diff/60001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_notification_manager.cc (right): https://codereview.chromium.org/1977423002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_notification_manager.cc:196: UMA_HISTOGRAM_COUNTS_100("PushMessaging.BackgroundBudget", budget); Your values are doubles, but histograms can only log integer values. Are you fine with the truncation? https://codereview.chromium.org/1977423002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1977423002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42374: +<histogram name="PushMessaging.BackgroundBudget"> Please add units= attribute for these. What units is the budget in? Bytes? Minutes? https://codereview.chromium.org/1977423002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42378: + available to the service worker. What does budget mean? Please explain.
PTAL https://codereview.chromium.org/1977423002/diff/60001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_notification_manager.cc (right): https://codereview.chromium.org/1977423002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_notification_manager.cc:196: UMA_HISTOGRAM_COUNTS_100("PushMessaging.BackgroundBudget", budget); On 2016/05/18 15:10:57, Alexei Svitkine wrote: > Your values are doubles, but histograms can only log integer values. Are you > fine with the truncation? Yes, the truncation is ok for our purposes. We expect the values to be distributed across the entire 0-100 spectrum and we're looking for general trends, not specific details. https://codereview.chromium.org/1977423002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1977423002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42374: +<histogram name="PushMessaging.BackgroundBudget"> On 2016/05/18 15:10:57, Alexei Svitkine wrote: > Please add units= attribute for these. What units is the budget in? Bytes? > Minutes? Unfortunately, it's just in "units". It's an internal measure that is being used to allocate some amount of work. In this first pass, the cost of various activities has been set arbitrarily, and we're hoping to use the information from the UMA to inform future costs. https://codereview.chromium.org/1977423002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42378: + available to the service worker. On 2016/05/18 15:10:57, Alexei Svitkine wrote: > What does budget mean? Please explain. I updated the description with additional information. Is that enough, or can you let me know what more you're looking for?
lgtm
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/1977423002/#ps80001 (title: "Updated histogram description")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977423002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977423002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1977423002/#ps100001 (title: "Post-rebase include fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977423002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977423002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by harkness@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977423002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977423002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Added UMA stats to track the budget for any service worker which receives a push notification and the SES score for service workers which have a low/no budget at the time of processing a push notification. This will be used to tune the heuristic for allowing service workers to do background processing without notifying the user every time. BUG=602244 ========== to ========== Added UMA stats to track the budget for any service worker which receives a push notification and the SES score for service workers which have a low/no budget at the time of processing a push notification. This will be used to tune the heuristic for allowing service workers to do background processing without notifying the user every time. BUG=602244 Committed: https://crrev.com/3f1dda7398a661f1c4e7834674bd450500007ff9 Cr-Commit-Position: refs/heads/master@{#394903} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3f1dda7398a661f1c4e7834674bd450500007ff9 Cr-Commit-Position: refs/heads/master@{#394903} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
