Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2760253003:
[Doodle] Record UMA for DoodleConfig download outcome and time (Closed)
|
Created:
3 years, 9 months ago by Marc Treib Modified:
3 years, 9 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Doodle] Record UMA for DoodleConfig download outcome and time
BUG=703227
Review-Url: https://codereview.chromium.org/2760253003
Cr-Commit-Position: refs/heads/master@{#459031}
Committed: https://chromium.googlesource.com/chromium/src/+/1732bbc476bb0c1a82bd79fbaa82250a2276d7d0
Patch Set 1 #
Total comments: 9
Patch Set 2 : review 1 #
Total comments: 4
Patch Set 3 : review 2 #
Total comments: 4
Patch Set 4 : review 3 #Patch Set 5 : tests #
Total comments: 5
Messages
Total messages: 31 (17 generated)
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
treib@chromium.org changed reviewers: + fhorschig@chromium.org
PTAL. WDYT about tests: Separate tests for metrics recording (but then I'll have to duplicate most of the existing tests), or checking metrics in the existing tests (but then, arguably, the tests will test more than one thing)?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Tests are a really difficult thing now, I agree. We have a lot of cases, so I would be fine with adding metric checks to existing test cases. If you manage to fit all metrics checks in just one or two tests, I would be even happier. Currently, I feel more strongly about the loooooooong UpdateCachedConfig. Maybe testing will become a little easier if we care about that first. https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:121: const base::Optional<DoodleConfig>& doodle_config) { 80 lines and nested ifs make this really hard to read. More below. https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:132: if (cached_config_ != new_config) { Can we consider putting this branch into a new function and replace the nested ifs with guard clauses? Outcome DoodleService::UpdateCache(new_config, TTL) https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:160: DCHECK(outcome != OUTCOME_COUNT); Is there any chance this couldn't have happened? https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:162: // Determine the final outcome. ERROR or EXPIRED override NO_DOODLE. So this complete branch is basically dead for one of two possible calls. Can we make it an own function? --> void ::RecordMetrics(outcome, state, download_time) Then you could return |outcome| in UpdateCachedConfig.
Partially addressed - is this better? I'll see if the UpdateCachedConfig method can be split up further. https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:132: if (cached_config_ != new_config) { On 2017/03/21 15:26:27, fhorschig wrote: > Can we consider putting this branch into a new function and replace the > nested ifs with guard clauses? > Outcome DoodleService::UpdateCache(new_config, TTL) Hm. Maaaybe there's a sensible way to split this up. I'll take a stab in the next patch set. https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:160: DCHECK(outcome != OUTCOME_COUNT); On 2017/03/21 15:26:27, fhorschig wrote: > Is there any chance this couldn't have happened? Nope, hence the DCHECK. https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:162: // Determine the final outcome. ERROR or EXPIRED override NO_DOODLE. On 2017/03/21 15:26:27, fhorschig wrote: > So this complete branch is basically dead for one of two possible calls. Good point. > Can we make it an own function? > --> void ::RecordMetrics(outcome, state, download_time) Done (mostly). I left the "switch(state)" here, because I don't want to split up the determination of outcome into two places. It's hard enough to follow as it is :) > Then you could return |outcome| in UpdateCachedConfig. Done.
That's the direction I had in mind! https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:162: // Determine the final outcome. ERROR or EXPIRED override NO_DOODLE. On 2017/03/21 16:10:45, Marc Treib wrote: > On 2017/03/21 15:26:27, fhorschig wrote: > > So this complete branch is basically dead for one of two possible calls. > > Good point. > > > Can we make it an own function? > > --> void ::RecordMetrics(outcome, state, download_time) > > Done (mostly). I left the "switch(state)" here, because I don't want to split up > the determination of outcome into two places. It's hard enough to follow as it > is :) > > > Then you could return |outcome| in UpdateCachedConfig. > > Done. Not dead but very unused for one call. that's only a little bit better. https://codereview.chromium.org/2760253003/diff/20001/components/doodle/doodl... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2760253003/diff/20001/components/doodle/doodl... components/doodle/doodle_service.h:68: enum DownloadOutcome { Question: could we use enum class DownloadOutcome: int {} here? If so, should we? (assigning fixed ints could reduce the danger of someone reordering them btw.) https://codereview.chromium.org/2760253003/diff/20001/components/doodle/doodl... components/doodle/doodle_service.h:80: static bool DownloadOutcomeIsSuccess(DownloadOutcome outcome); It's static because it couldn't access DownloadOutcome otherwise?
All better now (I hope). PTAL again! https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2760253003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:162: // Determine the final outcome. ERROR or EXPIRED override NO_DOODLE. On 2017/03/21 16:41:34, fhorschig wrote: > On 2017/03/21 16:10:45, Marc Treib wrote: > > On 2017/03/21 15:26:27, fhorschig wrote: > > > So this complete branch is basically dead for one of two possible calls. > > > > Good point. > > > > > Can we make it an own function? > > > --> void ::RecordMetrics(outcome, state, download_time) > > > > Done (mostly). I left the "switch(state)" here, because I don't want to split > up > > the determination of outcome into two places. It's hard enough to follow as it > > is :) > > > > > Then you could return |outcome| in UpdateCachedConfig. > > > > Done. > > Not dead but very unused for one call. > that's only a little bit better. Pulled it all out into a separate DetermineDownloadOutcome method. It's still unused in some cases, but IMO that's fine. https://codereview.chromium.org/2760253003/diff/20001/components/doodle/doodl... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2760253003/diff/20001/components/doodle/doodl... components/doodle/doodle_service.h:68: enum DownloadOutcome { On 2017/03/21 16:41:34, fhorschig wrote: > Question: could we use enum class DownloadOutcome: int {} here? Yes. > If so, should we? No, because enum class doesn't work with UMA_HISTOGRAM_ENUMERATION (we'd have to static_cast to int and use UMA_HISTOGRAM_LINEAR_COUNTS.. ugh). > (assigning fixed ints could reduce the danger of someone reordering them btw.) Sure, done. https://codereview.chromium.org/2760253003/diff/20001/components/doodle/doodl... components/doodle/doodle_service.h:80: static bool DownloadOutcomeIsSuccess(DownloadOutcome outcome); On 2017/03/21 16:41:34, fhorschig wrote: > It's static because it couldn't access DownloadOutcome otherwise? It's a *member* because otherwise it couldn't see DownloadOutcome. It's static because it doesn't need instance state. </smartass> (scnr :P)
I am way more comfortable with that :) https://codereview.chromium.org/2760253003/diff/40001/components/doodle/doodl... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2760253003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.cc:131: if (old_config.value() == new_config.value()) { Wait, why is that if in an else and not another else if? Am I missing something or is that just to show a separation between checking optionals and checking their values? https://codereview.chromium.org/2760253003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.cc:137: DCHECK_NE(outcome, OUTCOME_COUNT); Nit: why not return every outcome= and just put a NOTREACHED here? Or check it somewhere else?
https://codereview.chromium.org/2760253003/diff/40001/components/doodle/doodl... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2760253003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.cc:131: if (old_config.value() == new_config.value()) { On 2017/03/21 17:09:00, fhorschig wrote: > Wait, why is that if in an else and not another else if? > Am I missing something or is that just to show a separation > between checking optionals and checking their values? No particular reason; I just found it slightly easier to read like this. Anyway, gone now. https://codereview.chromium.org/2760253003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.cc:137: DCHECK_NE(outcome, OUTCOME_COUNT); On 2017/03/21 17:09:00, fhorschig wrote: > Nit: > why not return every outcome= and just put a NOTREACHED here? > Or check it somewhere else? Good point - this was just a leftover from moving code around. Done. Nice side effect: This gets rid of all the "else if"s :)
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
aaand added some tests. PTAL one (hopefully) final time :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, lgtm w/ nits. Nice to see that you wrote separate tests! https://codereview.chromium.org/2760253003/diff/80001/components/doodle/doodl... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2760253003/diff/80001/components/doodle/doodl... components/doodle/doodle_service.h:91: void DoodleFetched(base::TimeTicks start_time, nit: const base::TimeTicks& ? (I guess like for TimeDelta, it's mostly an int) https://codereview.chromium.org/2760253003/diff/80001/components/doodle/doodl... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2760253003/diff/80001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:371: // Metrics should have been recorded. nitty nit: This is pretty obvious ;) The later comments are fine because they describe the recording reason. https://codereview.chromium.org/2760253003/diff/80001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:390: // Metrics should have been recorded. The obvious comment nit again.
https://codereview.chromium.org/2760253003/diff/80001/components/doodle/doodl... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2760253003/diff/80001/components/doodle/doodl... components/doodle/doodle_service.h:91: void DoodleFetched(base::TimeTicks start_time, On 2017/03/22 09:47:10, fhorschig wrote: > nit: const base::TimeTicks& ? > (I guess like for TimeDelta, it's mostly an int) Yup, all the Time types are just an int64, so they're fine to pass by value. https://codereview.chromium.org/2760253003/diff/80001/components/doodle/doodl... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2760253003/diff/80001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:371: // Metrics should have been recorded. On 2017/03/22 09:47:10, fhorschig wrote: > nitty nit: This is pretty obvious ;) > The later comments are fine because they describe the recording reason. True, but I still like it to visually separate the "do the action" phase from "verify the outcome".
treib@chromium.org changed reviewers: + isherman@chromium.org
+isherman for histograms.xml, once again. PTAL, thanks!
Metrics LGTM, thanks.
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490258059627160,
"parent_rev": "3246896976ad46a095ef679b2421528536848059", "commit_rev":
"1732bbc476bb0c1a82bd79fbaa82250a2276d7d0"}
Message was sent while issue was closed.
Description was changed from ========== [Doodle] Record UMA for DoodleConfig download outcome and time BUG=703227 ========== to ========== [Doodle] Record UMA for DoodleConfig download outcome and time BUG=703227 Review-Url: https://codereview.chromium.org/2760253003 Cr-Commit-Position: refs/heads/master@{#459031} Committed: https://chromium.googlesource.com/chromium/src/+/1732bbc476bb0c1a82bd79fbaa82... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1732bbc476bb0c1a82bd79fbaa82... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
