Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1368533004:
Add UMA metrics to the site engagement service. (Closed)
|
Created:
5 years, 2 months ago by dominickn Modified:
5 years, 2 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@time-on-site Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA metrics to the site engagement service.
This CL adds UMA for a number of different statistics, including:
* total cumulative engagement and number of origins
* total origins which reach the capped daily and total engagement scores,
and the percentage of all origins per user which reach the total max
* total number of events which increase the engagement score,
differentiated by type
* engagement scores per origin
* mean and median engagement scores per user over all origins
Most statistics are recorded at non-incognito profile launch, and then
once per hour after that. Engagement type statistics are recorded per
engagement event. Tests are added to verify this behaviour.
BUG=464234
Committed: https://crrev.com/7fddcec708c1447c455d439e40cbb7e64b6aa5e5
Cr-Commit-Position: refs/heads/master@{#351973}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Addressing reviewer comments #
Total comments: 12
Patch Set 3 : Rebase #Patch Set 4 : Rebase #
Total comments: 2
Patch Set 5 : Syntax error on Windows #Patch Set 6 : Refactor addressing reviewer comments #
Total comments: 6
Patch Set 7 : Adding tests, addressing reviewer feedback #
Total comments: 23
Patch Set 8 : Addressing reviewer feedback, redesigning metrics, updating tests #Patch Set 9 : Rebase #
Total comments: 10
Patch Set 10 : Addressing reviewer feedback. Adding more testing #
Total comments: 4
Patch Set 11 : Fix compile error in browser tests #Patch Set 12 : Refactor tests #Dependent Patchsets: Messages
Total messages: 30 (6 generated)
dominickn@chromium.org changed reviewers: + benwells@chromium.org, calamity@chromium.org
PTAL. I'm not 100% sure I've used all of the correct histogram macros. Also happy to talk about the metrics I've used and when they're fired. Thanks!
Initial run. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_helper.cc:166: SiteEngagementMetrics::ENGAGEMENT_NAVIGATION); I think we should do this in HandleNavigation() in case we decide to separate different navigations. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_metrics.cc (right): https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_metrics.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY("SiteEngagementService.EngagementType", So what does SPARSE_SLOWLY do? https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_metrics.cc:46: UMA_HISTOGRAM_SPARSE_SLOWLY("SiteEngagementService.EngagementType", Pull into a local constant. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:254: OriginsWithMaxDailyEngagement()); Hmm. It's kind of weird to log this each time a score gets to the max. We'll get a triangle effect to account for. I'm not entirely sure what a nice way around this is... I think it's customary to either count one event each it happens (and therefore lose distribution data) or to add a 'last_logged' thing to prefs and then log if more than a day has passed since the last report was sent. I'm wary about doing that sort of logging though because if we complicate the decay function at all, we might get weird phase effects in our data.... Maybe just keep it simple and log once per event, WDYT? https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:261: OriginsWithMaxEngagement()); Ditto. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:270: std::string(), &engagement_settings); Worth factoring out a GetSiteEngagementSettings at this point I think. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.h:143: // maximum limits for the day and overall. nit: This should be moved into the previous CL but don't worry about it if it's too annoying. https://codereview.chromium.org/1368533004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1368533004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42168: + The cumulative total site engagement accumulated by this profile over all I would be very careful with calling this CumulativeTotal. This could all be misinterpreted as the total number of points awarded ever (ie not accounting for decay). Perhaps just TotalEngagementPoints would be sufficient. https://codereview.chromium.org/1368533004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42177: + The site engagement per origin accumulated by this profile over all time, EngagementScoreByOrigin, perhaps? // Counts the engagement score of each origin, recorded on non-incognito profile startup.
Thanks! https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_helper.cc:166: SiteEngagementMetrics::ENGAGEMENT_NAVIGATION); On 2015/09/25 07:08:33, calamity wrote: > I think we should do this in HandleNavigation() in case we decide to separate > different navigations. It would be inconsistent to have the user input UMA logged in the helper but the navigation UMA logged in the service. I think if we split out different navigations we can simply do like the user input UMA does here and directly log the transition type in the helper rather than sending it into the service. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_metrics.cc (right): https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_metrics.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY("SiteEngagementService.EngagementType", On 2015/09/25 07:08:33, calamity wrote: > So what does SPARSE_SLOWLY do? asvitkine recommend that I use it instead of UMA_HISTOGRAM_CUSTOM_ENUMERATION in an app banner CL a couple of months ago, so I stuck with it here for the enum histogram. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_metrics.cc:46: UMA_HISTOGRAM_SPARSE_SLOWLY("SiteEngagementService.EngagementType", On 2015/09/25 07:08:33, calamity wrote: > Pull into a local constant. Done. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:254: OriginsWithMaxDailyEngagement()); On 2015/09/25 07:08:33, calamity wrote: > Hmm. It's kind of weird to log this each time a score gets to the max. We'll get > a triangle effect to account for. I'm not entirely sure what a nice way around > this is... > > I think it's customary to either count one event each it happens (and therefore > lose distribution data) or to add a 'last_logged' thing to prefs and then log if > more than a day has passed since the last report was sent. I'm wary about doing > that sort of logging though because if we complicate the decay function at all, > we might get weird phase effects in our data.... > > Maybe just keep it simple and log once per event, WDYT? I wanted to try and avoid the overhead of finding all of the maxed out origins per each increment in engagement, which is why I only logged this when a score reached the maximum. I agree that having a last_logged date in prefs is undesirable. What I'm aiming for with this statistic is having a histogram of the total number of origins with max engagement per client. By logging the total origins every time an origin reaches the max, you get a CDF where each increasing number of origins is a subset of all of the previous numbers (putting aside when people clear the prefs). Does that make sense? https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:261: OriginsWithMaxEngagement()); On 2015/09/25 07:08:33, calamity wrote: > Ditto. Acknowledged. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:270: std::string(), &engagement_settings); On 2015/09/25 07:08:33, calamity wrote: > Worth factoring out a GetSiteEngagementSettings at this point I think. I see you've done it in your cleanup CL: I'll wait for that to land and rebase this change on it (also addressing the PostTask so we avoid a double iteration over the score map). https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.h:143: // maximum limits for the day and overall. On 2015/09/25 07:08:33, calamity wrote: > nit: This should be moved into the previous CL but don't worry about it if it's > too annoying. Done. https://codereview.chromium.org/1368533004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1368533004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42168: + The cumulative total site engagement accumulated by this profile over all On 2015/09/25 07:08:33, calamity wrote: > I would be very careful with calling this CumulativeTotal. This could all be > misinterpreted as the total number of points awarded ever (ie not accounting for > decay). > > Perhaps just TotalEngagementPoints would be sufficient. Done. https://codereview.chromium.org/1368533004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42177: + The site engagement per origin accumulated by this profile over all time, On 2015/09/25 07:08:33, calamity wrote: > EngagementScoreByOrigin, perhaps? > > // Counts the engagement score of each origin, recorded on non-incognito profile > startup. Done.
https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_helper.cc:166: SiteEngagementMetrics::ENGAGEMENT_NAVIGATION); On 2015/09/28 03:34:50, dominickn wrote: > On 2015/09/25 07:08:33, calamity wrote: > > I think we should do this in HandleNavigation() in case we decide to separate > > different navigations. > > It would be inconsistent to have the user input UMA logged in the helper but the > navigation UMA logged in the service. I think if we split out different > navigations we can simply do like the user input UMA does here and directly log > the transition type in the helper rather than sending it into the service. Hmm, I'd prefer all our logging take place in the same place. In fact, even the engagement type above ought to be passed into the SiteEngagementService. I think this class should remain as much as possible a pure input to SiteEngagementService rather than something that thinks about any sort of real logic. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_metrics.cc (right): https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_metrics.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY("SiteEngagementService.EngagementType", On 2015/09/28 03:34:50, dominickn wrote: > On 2015/09/25 07:08:33, calamity wrote: > > So what does SPARSE_SLOWLY do? > > asvitkine recommend that I use it instead of UMA_HISTOGRAM_CUSTOM_ENUMERATION in > an app banner CL a couple of months ago, so I stuck with it here for the enum > histogram. Acknowledged. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:254: OriginsWithMaxDailyEngagement()); On 2015/09/28 03:34:50, dominickn wrote: > On 2015/09/25 07:08:33, calamity wrote: > > Hmm. It's kind of weird to log this each time a score gets to the max. We'll > get > > a triangle effect to account for. I'm not entirely sure what a nice way around > > this is... > > > > I think it's customary to either count one event each it happens (and > therefore > > lose distribution data) or to add a 'last_logged' thing to prefs and then log > if > > more than a day has passed since the last report was sent. I'm wary about > doing > > that sort of logging though because if we complicate the decay function at > all, > > we might get weird phase effects in our data.... > > > > Maybe just keep it simple and log once per event, WDYT? > > I wanted to try and avoid the overhead of finding all of the maxed out origins > per each increment in engagement, which is why I only logged this when a score > reached the maximum. I agree that having a last_logged date in prefs is > undesirable. > > What I'm aiming for with this statistic is having a histogram of the total > number of origins with max engagement per client. By logging the total origins > every time an origin reaches the max, you get a CDF where each increasing number > of origins is a subset of all of the previous numbers (putting aside when people > clear the prefs). Does that make sense? Ok, I'll defer to the histograms reviewer on this. Also, the histogram comment ought to make note of this since it's fairly unusual. Just as a note, the UMA system has a CDF column which will not be the actual CDF for the stat. https://codereview.chromium.org/1368533004/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:270: std::string(), &engagement_settings); On 2015/09/28 03:34:50, dominickn wrote: > On 2015/09/25 07:08:33, calamity wrote: > > Worth factoring out a GetSiteEngagementSettings at this point I think. > > I see you've done it in your cleanup CL: I'll wait for that to land and rebase > this change on it (also addressing the PostTask so we avoid a double iteration > over the score map). But that's based on your time on site stuff! Ugh, we're getting tangled. =( https://codereview.chromium.org/1368533004/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_metrics.cc (right): https://codereview.chromium.org/1368533004/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_metrics.cc:28: } // anonymous namespace nit: Just // namespace. (Matches rest of Chrome) https://codereview.chromium.org/1368533004/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_metrics.cc:32: UMA_HISTOGRAM_COUNTS_100(kTotalEngagementHistogram, total_engagement); Is this a histogram of max value 100? You're gonna need a bigger boat. https://codereview.chromium.org/1368533004/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_metrics.cc:36: UMA_HISTOGRAM_COUNTS_100(kTotalOriginsHistogram, num_origins); Ditto. https://codereview.chromium.org/1368533004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1368533004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42188: + profile, once per non-incognito startup. ...recorded once per... https://codereview.chromium.org/1368533004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42196: + The number of origins which have reached the capped maximum site engagement The number of origins which have reached the daily site engagement point cap https://codereview.chromium.org/1368533004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42205: + The number of origins which reach the capped maximum site engagement total, The number of origins which have reached the absolute site engagement point cap https://codereview.chromium.org/1368533004/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/60001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:261: OriginsWithMaxEngagement()); Hmm. Does this mean we will be re-recording these stats for maxed origins? e.g an origin reaches 100, the next input comes in and triggers this code again. We need to keep track of the old value and only do this on a transition.
Thanks! https://codereview.chromium.org/1368533004/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_metrics.cc (right): https://codereview.chromium.org/1368533004/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_metrics.cc:28: } // anonymous namespace On 2015/09/28 07:33:47, calamity wrote: > nit: Just // namespace. (Matches rest of Chrome) Done. https://codereview.chromium.org/1368533004/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_metrics.cc:32: UMA_HISTOGRAM_COUNTS_100(kTotalEngagementHistogram, total_engagement); On 2015/09/28 07:33:47, calamity wrote: > Is this a histogram of max value 100? You're gonna need a bigger boat. Done. https://codereview.chromium.org/1368533004/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_metrics.cc:36: UMA_HISTOGRAM_COUNTS_100(kTotalOriginsHistogram, num_origins); On 2015/09/28 07:33:48, calamity wrote: > Ditto. Done. https://codereview.chromium.org/1368533004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1368533004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42188: + profile, once per non-incognito startup. On 2015/09/28 07:33:48, calamity wrote: > ...recorded once per... Done. https://codereview.chromium.org/1368533004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42196: + The number of origins which have reached the capped maximum site engagement On 2015/09/28 07:33:48, calamity wrote: > The number of origins which have reached the daily site engagement point cap Done. https://codereview.chromium.org/1368533004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42205: + The number of origins which reach the capped maximum site engagement total, On 2015/09/28 07:33:48, calamity wrote: > The number of origins which have reached the absolute site engagement point cap Done. https://codereview.chromium.org/1368533004/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/60001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:261: OriginsWithMaxEngagement()); On 2015/09/28 07:33:48, calamity wrote: > Hmm. Does this mean we will be re-recording these stats for maxed origins? > > e.g an origin reaches 100, the next input comes in and triggers this code again. > > We need to keep track of the old value and only do this on a transition. Done.
lgtm https://codereview.chromium.org/1368533004/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:260: if (score.MaxPointsPerDayAdded()) { Re-record issue also exists here I believe. https://codereview.chromium.org/1368533004/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:308: std::map<GURL, double> score_map = GetScoreMap(); Just NB that this should be run after the cleanup in my other CL. https://codereview.chromium.org/1368533004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1368533004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:42235: + non-incognito startup. Add some information to these 2 summaries about the CDF behavior.
Sorry if this has been asked already, but why are the number of max engagements being recorded like this? Why not just at startup? Also, the 'by origin' UMA is weird. We're not recording anything by origin as it would be a privacy violation, so I'm confused by the naming.
On 2015/09/29 04:39:52, benwells wrote: > Sorry if this has been asked already, but why are the number of max engagements > being recorded like this? Why not just at startup? A few reasons. I wanted to avoid queueing effects (origins hit the maximum, but have decayed by the next time the user restarts). I also wanted to avoid overhead and skew: it should take a while before anyone hits the maximum, so there will be many 0 samples in the ramp up. > Also, the 'by origin' UMA is weird. We're not recording anything by origin as it > would be a privacy violation, so I'm confused by the naming. I'll rename the by origin UMA to make it clearer that it's recording the engagement distribution, and no PII.
https://codereview.chromium.org/1368533004/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:260: if (score.MaxPointsPerDayAdded()) { On 2015/09/29 04:21:09, calamity wrote: > Re-record issue also exists here I believe. Acknowledged. https://codereview.chromium.org/1368533004/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:308: std::map<GURL, double> score_map = GetScoreMap(); On 2015/09/29 04:21:09, calamity wrote: > Just NB that this should be run after the cleanup in my other CL. Acknowledged. https://codereview.chromium.org/1368533004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1368533004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:42235: + non-incognito startup. On 2015/09/29 04:21:09, calamity wrote: > Add some information to these 2 summaries about the CDF behavior. Done.
I'm still unsure about the way max engagements are being recorded. Can you explained how the numbers will be interpreted, as that's what I'm really worried about... https://codereview.chromium.org/1368533004/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_metrics.cc (right): https://codereview.chromium.org/1368533004/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_metrics.cc:57: UMA_HISTOGRAM_SPARSE_SLOWLY(kEngagementTypeHistogram, Reading https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/f-fO6I5UleU I think this is the wrong macro to use. https://codereview.chromium.org/1368533004/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_metrics.h (right): https://codereview.chromium.org/1368533004/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_metrics.h:25: static void RecordTotalOriginsEngaged(int total_origins); Nit: I don' think you need a blank line between every function. It's good to group related things, you could easily say these are all related so should all be grouped. Or maybe the Max ones are a group. https://codereview.chromium.org/1368533004/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_metrics.h:27: static void RecordEngagementScore(std::map<GURL, double> score_map); Nit: this should be RecordEngagementScores (i.e. with an S at the end). https://codereview.chromium.org/1368533004/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_metrics.h:35: static const char kTotalEngagementHistogram[]; Nit: normally you'd put these first (and that would match the .cc file order). https://codereview.chromium.org/1368533004/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_metrics.h:41: Nit: no blank line. https://codereview.chromium.org/1368533004/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:108: points_added_today_ = 0; Why is this updated? When originally writing this code I tried to make the internals not update unless they were going to be written to disk to make it simpler to reason about. https://codereview.chromium.org/1368533004/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:274: int SiteEngagementService::OriginsWithMaxDailyEngagement() { Is there a reason not to use GetScoreMap to implement this? Its strange that you updated GetTotalEngagementPoints to use GetScoreMap but don't use it here.
dominickn@chromium.org changed reviewers: + isherman@chromium.org
isherman: plkease review tools/, and advise with the appropriate design/macros for histograms. Thanks!
https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... File chrome/browser/engagement/site_engagement_metrics.h (right): https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_metrics.h:25: static void RecordTotalOriginsEngaged(int total_origins); On 2015/09/30 00:10:54, benwells wrote: > Nit: I don' think you need a blank line between every function. It's good to > group related things, you could easily say these are all related so should all > be grouped. Or maybe the Max ones are a group. Done. https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_metrics.h:27: static void RecordEngagementScore(std::map<GURL, double> score_map); On 2015/09/30 00:10:54, benwells wrote: > Nit: this should be RecordEngagementScores (i.e. with an S at the end). Done. https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_metrics.h:35: static const char kTotalEngagementHistogram[]; On 2015/09/30 00:10:54, benwells wrote: > Nit: normally you'd put these first (and that would match the .cc file order). Done. https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_metrics.h:41: On 2015/09/30 00:10:54, benwells wrote: > Nit: no blank line. Done. https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... File chrome/browser/engagement/site_engagement_service.cc (right): https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_service.cc:108: points_added_today_ = 0; On 2015/09/30 00:10:54, benwells wrote: > Why is this updated? When originally writing this code I tried to make the > internals not update unless they were going to be written to disk to make it > simpler to reason about. I added this update to ensure that day boundary effects didn't give false positives when recording the number of maxed daily origins. This is the only place where this attribute is read without the score being incremented as well (and hence written). Do you think that the boundary effect is trivial enough to skip this check entirely? https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_service.cc:274: int SiteEngagementService::OriginsWithMaxDailyEngagement() { On 2015/09/30 00:10:54, benwells wrote: > Is there a reason not to use GetScoreMap to implement this? Its strange that you > updated GetTotalEngagementPoints to use GetScoreMap but don't use it here. The main reason was to avoid a double-loop over all of the origins: one to construct the score map, and the second to iterate over it again and check all the origins for maxed daily scores. If you think this cost is okay, then I'm happy to use GetScoreMap.
I think the metric design is plausible, but I agree that it's not completely clear. https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... File chrome/browser/engagement/site_engagement_metrics.cc (right): https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_metrics.cc:57: UMA_HISTOGRAM_SPARSE_SLOWLY(kEngagementTypeHistogram, On 2015/09/30 00:10:54, benwells wrote: > Reading > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/f-fO6I5UleU > I think this is the wrong macro to use. It's fine either way, but yeah, UMA_HISTOGRAM_ENUMERATION is probably a little better suited for this tiny enum. https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_metrics.cc:65: UMA_HISTOGRAM_SPARSE_SLOWLY(kEngagementTypeHistogram, ENGAGEMENT_MOUSE); nit: Please move the UMA_HISTOGRAM_ macro out of the switch stmt, so that the switch stmt just sets the metric that should be emitted. (i.e., please don't repeat this line three times.) https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_metrics.cc:67: default: nit: Please remove the default case: the compiler should be able to enforce that all cases are covered, but only if you remove the default case. https://chromiumcodereview.appspot.com/1368533004/diff/120001/tools/metrics/h... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/1368533004/diff/120001/tools/metrics/h... tools/metrics/histograms/histograms.xml:42189: + per non-incognito startup. What is an Incognito startup? Does "startup" mean something other than browser process launch in this context? https://chromiumcodereview.appspot.com/1368533004/diff/120001/tools/metrics/h... tools/metrics/histograms/histograms.xml:42219: + daily site engagement point cap. When does N reset -- upon browser relaunch? And, is zero never recorded for this histogram?
Thanks for the feedback! https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... File chrome/browser/engagement/site_engagement_metrics.cc (right): https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_metrics.cc:57: UMA_HISTOGRAM_SPARSE_SLOWLY(kEngagementTypeHistogram, On 2015/10/01 07:08:17, Ilya Sherman wrote: > On 2015/09/30 00:10:54, benwells wrote: > > Reading > > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/f-fO6I5UleU > > I think this is the wrong macro to use. > > It's fine either way, but yeah, UMA_HISTOGRAM_ENUMERATION is probably a little > better suited for this tiny enum. Done. https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_metrics.cc:65: UMA_HISTOGRAM_SPARSE_SLOWLY(kEngagementTypeHistogram, ENGAGEMENT_MOUSE); On 2015/10/01 07:08:17, Ilya Sherman wrote: > nit: Please move the UMA_HISTOGRAM_ macro out of the switch stmt, so that the > switch stmt just sets the metric that should be emitted. (i.e., please don't > repeat this line three times.) Done. https://chromiumcodereview.appspot.com/1368533004/diff/120001/chrome/browser/... chrome/browser/engagement/site_engagement_metrics.cc:67: default: On 2015/10/01 07:08:17, Ilya Sherman wrote: > nit: Please remove the default case: the compiler should be able to enforce that > all cases are covered, but only if you remove the default case. Done. https://chromiumcodereview.appspot.com/1368533004/diff/120001/tools/metrics/h... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/1368533004/diff/120001/tools/metrics/h... tools/metrics/histograms/histograms.xml:42189: + per non-incognito startup. On 2015/10/01 07:08:17, Ilya Sherman wrote: > What is an Incognito startup? Does "startup" mean something other than browser > process launch in this context? I have clarified this as "once per startup per non-incognito profile launch". https://chromiumcodereview.appspot.com/1368533004/diff/120001/tools/metrics/h... tools/metrics/histograms/histograms.xml:42219: + daily site engagement point cap. On 2015/10/01 07:08:17, Ilya Sherman wrote: > When does N reset -- upon browser relaunch? > > And, is zero never recorded for this histogram? Upon discussion, we have redesigned this histogram so that it and several others are recorded at browser startup per non-incognito profile, and then every hour hence. This should address the zero issue, though there is still some skew against users who do not browse for long period of time.
Patchset #9 (id:160001) has been deleted
Patchset #9 (id:180001) has been deleted
lgtm with minor comments https://chromiumcodereview.appspot.com/1368533004/diff/200001/chrome/browser/... File chrome/browser/engagement/site_engagement_service.cc (right): https://chromiumcodereview.appspot.com/1368533004/diff/200001/chrome/browser/... chrome/browser/engagement/site_engagement_service.cc:215: if (clock_->Now() - last_metrics_time_ >= metrics_interval) This could happen before the startup stuff. You should make this check handle last_metrics_time_ being null, and also move the check into RecordMetrics so it is only done once. https://chromiumcodereview.appspot.com/1368533004/diff/200001/chrome/browser/... File chrome/browser/engagement/site_engagement_service.h (right): https://chromiumcodereview.appspot.com/1368533004/diff/200001/chrome/browser/... chrome/browser/engagement/site_engagement_service.h:63: // the number of points added today if the day has changed. Nit: you can delete the last sentence now. https://chromiumcodereview.appspot.com/1368533004/diff/200001/chrome/browser/... chrome/browser/engagement/site_engagement_service.h:166: Nit: only one blank line.
LGTM, thanks.
slgtm https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_metrics.h (right): https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_metrics.h:32: static const char kEngagementTypeHistogram[]; Can these just live in the anonymous namespace? https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:329: total_origins += 1; nit: ++total_origins
Thanks for the comments! This final version addresses nits, adds the median statistic and bulks out the histogram testing. https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_metrics.h (right): https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_metrics.h:32: static const char kEngagementTypeHistogram[]; On 2015/10/02 03:46:28, calamity wrote: > Can these just live in the anonymous namespace? No, because they're used in tests. I've privatized them. https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:215: if (clock_->Now() - last_metrics_time_ >= metrics_interval) On 2015/10/02 01:33:25, benwells wrote: > This could happen before the startup stuff. You should make this check handle > last_metrics_time_ being null, and also move the check into RecordMetrics so it > is only done once. Done. https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:329: total_origins += 1; On 2015/10/02 03:46:29, calamity wrote: > nit: ++total_origins Done. https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:63: // the number of points added today if the day has changed. On 2015/10/02 01:33:25, benwells wrote: > Nit: you can delete the last sentence now. Done. https://codereview.chromium.org/1368533004/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:166: On 2015/10/02 01:33:25, benwells wrote: > Nit: only one blank line. Done.
https://codereview.chromium.org/1368533004/diff/220001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/220001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:354: std::sort(scores.begin(), scores.end()); Consider using std::nth_element to compute the median. https://codereview.chromium.org/1368533004/diff/220001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/1368533004/diff/220001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:714: SiteEngagementMetrics::ENGAGEMENT_KEYPRESS, 3); These tests are getting too unruly. Can you just factor out a separate histograms test that just checks each histogram event simply and then one that checks the 1 hour interval thing. I think if these tests fail, it's going to be confusing to figure out where and why things are going wrong. It'd be nicer to have a single test that will fail for histograms stuff that is divorced as possible from other functionality. In general, it would be good to have specific tests that don't overtest the same codepaths.
Thanks! https://codereview.chromium.org/1368533004/diff/220001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1368533004/diff/220001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:354: std::sort(scores.begin(), scores.end()); On 2015/10/02 05:31:18, calamity wrote: > Consider using std::nth_element to compute the median. I think this is a bit too micro-optimisation-ish to implement, plus it'll need 2 calls to nth_element (and an extra variable) to compute the even sized list case. https://codereview.chromium.org/1368533004/diff/220001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/1368533004/diff/220001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:714: SiteEngagementMetrics::ENGAGEMENT_KEYPRESS, 3); On 2015/10/02 05:31:18, calamity wrote: > These tests are getting too unruly. Can you just factor out a separate > histograms test that just checks each histogram event simply and then one that > checks the 1 hour interval thing. > > I think if these tests fail, it's going to be confusing to figure out where and > why things are going wrong. It'd be nicer to have a single test that will fail > for histograms stuff that is divorced as possible from other functionality. > > In general, it would be good to have specific tests that don't overtest the same > codepaths. Done.
lgtm
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benwells@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1368533004/#ps260001 (title: "Refactor tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1368533004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1368533004/260001
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/7fddcec708c1447c455d439e40cbb7e64b6aa5e5 Cr-Commit-Position: refs/heads/master@{#351973} |
