Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2949173002:
UMA for the Stale Previews timestamp (Closed)
DescriptionUMA for the Stale Previews timestamp
Add UMA for whether the timestamp for a stale preview was
shown on the infobar. If the timestamp was not shown, states
the reason why.
BUG=718643
Review-Url: https://codereview.chromium.org/2949173002
Cr-Commit-Position: refs/heads/master@{#482009}
Committed: https://chromium.googlesource.com/chromium/src/+/e45bad32544069664781ca144ee7cdc96cb58490
Patch Set 1 #Patch Set 2 : update tests #
Total comments: 13
Patch Set 3 : tbansal comments #Patch Set 4 : rebase #Patch Set 5 : test fix #
Total comments: 4
Patch Set 6 : comments #
Dependent Patchsets: Messages
Total messages: 43 (31 generated)
Description was changed from ========== UMA for the Stale Previews timestamp BUG=718667 ========== to ========== UMA for the Stale Previews timestamp BUG=718643 ==========
The CQ bit was checked by megjablon@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...
Description was changed from ========== UMA for the Stale Previews timestamp BUG=718643 ========== to ========== UMA for the Stale Previews timestamp Add uma for whether the timestamp for a stale preview was shown on the infobar. If the timestamp was not shown, states the reason why. BUG=718643 ==========
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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.
The CQ bit was checked by megjablon@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...
Description was changed from ========== UMA for the Stale Previews timestamp Add uma for whether the timestamp for a stale preview was shown on the infobar. If the timestamp was not shown, states the reason why. BUG=718643 ========== to ========== UMA for the Stale Previews timestamp Add UMA for whether the timestamp for a stale preview was shown on the infobar. If the timestamp was not shown, states the reason why. BUG=718643 ==========
megjablon@chromium.org changed reviewers: + tbansal@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:266: RecordStaleness(TIMESTAMP_NOT_SHOWN_PARAMS_NOT_AVAILABLE); Can this be changed to: DCHECK(min_staleness_in_minutes > 0 && max_staleness_in_minutes > 0); because if the experiment is enabled, then the config should really also be providing the experiment params. https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:276: network_time = base::Time::Now(); Record separate UMA here? I guess if the time tracker has low availability, then we need to come up with a custom solution? https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:294: RecordStaleness(TIMESTAMP_SHOWN); Do we want to record exactly which text (among three options) was shown? https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate_unittest.cc:187: std::map<std::string, std::string> variation_params) { #inclide <map> https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate_unittest.cc:188: field_trial_list_.reset(); Is this needed given the following line is also calling reset? https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate_unittest.cc:771: TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampMaximumUMA) { Would it be useful to combine this test with the ones above? We can iterate over a struct that stores |staleness_in_minutes| and |expected_unique_sample|?
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by megjablon@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 checked by megjablon@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...
https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:266: RecordStaleness(TIMESTAMP_NOT_SHOWN_PARAMS_NOT_AVAILABLE); On 2017/06/22 21:12:16, tbansal1 wrote: > Can this be changed to: > DCHECK(min_staleness_in_minutes > 0 && max_staleness_in_minutes > 0); > > because if the experiment is enabled, then the config should really also be > providing the experiment params. Added NOTREACHED(); since we should still return on non-debug builds. https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:276: network_time = base::Time::Now(); On 2017/06/22 21:12:16, tbansal1 wrote: > Record separate UMA here? I guess if the time tracker has low availability, then > we need to come up with a custom solution? I can add that separately if we decide we need it. I'm still going to investigate the time tracker more later, but want to get all this code in before feature freeze. https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:294: RecordStaleness(TIMESTAMP_SHOWN); On 2017/06/22 21:12:16, tbansal1 wrote: > Do we want to record exactly which text (among three options) was shown? We can, but if we just care about the number of stale previews being seen and how old they are, we can get those numbers from the server. https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate_unittest.cc:187: std::map<std::string, std::string> variation_params) { On 2017/06/22 21:12:16, tbansal1 wrote: > #inclide <map> Done. https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate_unittest.cc:188: field_trial_list_.reset(); On 2017/06/22 21:12:16, tbansal1 wrote: > Is this needed given the following line is also calling reset? It's needed because otherwise the constructor throws an error: [field_trial.cc(500)] Check failed: !global_. https://cs.chromium.org/chromium/src/base/metrics/field_trial.cc?rcl=d795cd03... https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate_unittest.cc:771: TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampMaximumUMA) { On 2017/06/22 21:12:16, tbansal1 wrote: > Would it be useful to combine this test with the ones above? > We can iterate over a struct that stores |staleness_in_minutes| and > |expected_unique_sample|? I refactored the tests. I didn't add a struct though because I heard we're trying to move away from using that in our tests. PTAL.
lgtm https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2949173002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate_unittest.cc:771: TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampMaximumUMA) { On 2017/06/22 23:40:11, megjablon wrote: > On 2017/06/22 21:12:16, tbansal1 wrote: > > Would it be useful to combine this test with the ones above? > > We can iterate over a struct that stores |staleness_in_minutes| and > > |expected_unique_sample|? > > I refactored the tests. I didn't add a struct though because I heard we're > trying to move away from using that in our tests. PTAL. Thanks, this looks much cleaner. https://codereview.chromium.org/2949173002/diff/120001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2949173002/diff/120001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate_unittest.cc:189: std::map<std::string, std::string> variation_params) { nit: pass as const ref. https://codereview.chromium.org/2949173002/diff/120001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate_unittest.cc:616: TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampMintuesTest) { s/Mintues/Minutes/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2949173002/#ps140001 (title: "comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by megjablon@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
megjablon@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow: PTAL histograms.xml https://codereview.chromium.org/2949173002/diff/120001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2949173002/diff/120001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate_unittest.cc:189: std::map<std::string, std::string> variation_params) { On 2017/06/23 00:37:42, tbansal1 wrote: > nit: pass as const ref. Done. https://codereview.chromium.org/2949173002/diff/120001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate_unittest.cc:616: TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampMintuesTest) { On 2017/06/23 00:37:42, tbansal1 wrote: > s/Mintues/Minutes/ Done.
lgtm histogram lg
The CQ bit was checked by megjablon@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": 140001, "attempt_start_ts": 1498249682371040,
"parent_rev": "e9fa8e854614147a142d4f7f1199510de7499949", "commit_rev":
"e45bad32544069664781ca144ee7cdc96cb58490"}
Message was sent while issue was closed.
Description was changed from ========== UMA for the Stale Previews timestamp Add UMA for whether the timestamp for a stale preview was shown on the infobar. If the timestamp was not shown, states the reason why. BUG=718643 ========== to ========== UMA for the Stale Previews timestamp Add UMA for whether the timestamp for a stale preview was shown on the infobar. If the timestamp was not shown, states the reason why. BUG=718643 Review-Url: https://codereview.chromium.org/2949173002 Cr-Commit-Position: refs/heads/master@{#482009} Committed: https://chromium.googlesource.com/chromium/src/+/e45bad32544069664781ca144ee7... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/e45bad32544069664781ca144ee7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
