Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2477073002:
Adding UMA to track previews opt outs and blacklist eligibility (Closed)
DescriptionAdding UMA to track previews opt outs and blacklist eligibility
This CL adds UMA to track the reason that a user was not shown a preview
(or if it was allowed) and whether a user opts out when shown a preview.
BUG=647717
Committed: https://crrev.com/cc61592651c3018a146387f691bc4cbd60f7807b
Committed: https://crrev.com/af5258ab4404766182e87f2c773051684c7d8c72
Cr-Original-Commit-Position: refs/heads/master@{#431020}
Cr-Commit-Position: refs/heads/master@{#431181}
Patch Set 1 #Patch Set 2 : removed general histogram #Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : rebase #
Total comments: 14
Patch Set 6 : tbansal comments #Patch Set 7 : build file change #
Total comments: 6
Patch Set 8 : tbansal comments #
Total comments: 2
Patch Set 9 : asvitkine nit #Patch Set 10 : failing DCHECK fixed in test #Patch Set 11 : tbansal nits #Patch Set 12 : typo #
Messages
Total messages: 64 (50 generated)
The CQ bit was checked by ryansturm@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.
The CQ bit was checked by ryansturm@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ryansturm@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 ryansturm@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: 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...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ryansturm@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...
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
tbansal: PTAL
https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... components/previews/core/previews_black_list.cc:79: UMA_HISTOGRAM_BOOLEAN("Previews.OptOut.UserOptedOut.Offline", opt_out); May be Previews.Offline.*.* so that all Offline histograms have same prefix. https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... components/previews/core/previews_black_list.h:33: enum class PreviewsEligibilityReason { nit: Is there a reason to use this style of comments vs. enum class A { // comment for VALUE1 VALUE1 = 0, // comment for VALUE2 ... }; I find the latter slightly more readable especially when the comment is multi-line. https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... File components/previews/core/previews_io_data.cc (right): https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... components/previews/core/previews_io_data.cc:32: "Previews.EligibilityReason.Offline", static_cast<int>(status), I would use Previews.Offline.EligibilityReason to ensure that all Offline histograms have same prefix (similar to PageLoad.Clients.DataReductionProxy.*). https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... components/previews/core/previews_io_data.cc:120: return true; Add: LogPreviewsEligibilityReason(PreviewsEligibilityReason::ALLOWED, type); before returning true? https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... components/previews/core/previews_io_data.cc:120: return true; Also, add a test to components/previews/core/previews_io_data_unittest.cc that checks for histogram correctness? https://codereview.chromium.org/2477073002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2477073002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48045: + enum="PreviewsEligibleReason"> s/PreviewsEligibleReason/PreviewsEligibilityReason/ to match the enum name used in the .cc code. https://codereview.chromium.org/2477073002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:95980: + <int value="2" label="Blacklist data not loaded yet from disk."/> "Blacklist" or "Black list"
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 ryansturm@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: 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...)
The CQ bit was checked by ryansturm@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...
tbansal: ptal https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... components/previews/core/previews_black_list.cc:79: UMA_HISTOGRAM_BOOLEAN("Previews.OptOut.UserOptedOut.Offline", opt_out); On 2016/11/08 00:30:52, tbansal1 wrote: > May be Previews.Offline.*.* so that all Offline histograms have same prefix. I was thinking of having a suffix for all of them with .Offline or .LoFi, etc. Similar to Previews.InfoBarAction.LoFi https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... components/previews/core/previews_black_list.h:33: enum class PreviewsEligibilityReason { On 2016/11/08 00:30:52, tbansal1 wrote: > nit: Is there a reason to use this style of comments vs. > enum class A { > // comment for VALUE1 > VALUE1 = 0, > > // comment for VALUE2 > ... > }; > > I find the latter slightly more readable especially when the comment is > multi-line. Done. https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... File components/previews/core/previews_io_data.cc (right): https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... components/previews/core/previews_io_data.cc:32: "Previews.EligibilityReason.Offline", static_cast<int>(status), On 2016/11/08 00:30:52, tbansal1 wrote: > I would use Previews.Offline.EligibilityReason to ensure that all Offline > histograms have same prefix (similar to PageLoad.Clients.DataReductionProxy.*). Like above, I was just going to suffix with the type (this doesn't make anything harder in a workflow AFAIK). https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... components/previews/core/previews_io_data.cc:120: return true; On 2016/11/08 00:30:52, tbansal1 wrote: > Add: > LogPreviewsEligibilityReason(PreviewsEligibilityReason::ALLOWED, type); > before returning true? Done. https://codereview.chromium.org/2477073002/diff/80001/components/previews/cor... components/previews/core/previews_io_data.cc:120: return true; On 2016/11/08 00:30:52, tbansal1 wrote: > Also, add a test to components/previews/core/previews_io_data_unittest.cc that > checks for histogram correctness? Done. https://codereview.chromium.org/2477073002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2477073002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48045: + enum="PreviewsEligibleReason"> On 2016/11/08 00:30:52, tbansal1 wrote: > s/PreviewsEligibleReason/PreviewsEligibilityReason/ > to match the enum name used in the .cc code. Done. https://codereview.chromium.org/2477073002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:95980: + <int value="2" label="Blacklist data not loaded yet from disk."/> On 2016/11/08 00:30:52, tbansal1 wrote: > "Blacklist" or "Black list" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % comments. https://codereview.chromium.org/2477073002/diff/120001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2477073002/diff/120001/components/previews/co... components/previews/core/previews_black_list.cc:9: #include "base/metrics/histogram.h" include base/metrics/histogram_macros.h instead of base/metrics/histogram.h? https://codereview.chromium.org/2477073002/diff/120001/components/previews/co... components/previews/core/previews_black_list.cc:79: UMA_HISTOGRAM_BOOLEAN("Previews.OptOut.UserOptedOut.Offline", opt_out); May be add a test for this? https://codereview.chromium.org/2477073002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2477073002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48334: +<histogram name="Previews.EligibilityReason.Offline" May be name this "Previews.EligibilityReason" and the one below "Previews.OptOut.UserOptedOut". Then create "Offline" suffix and apply it to both histograms. Makes it easier to add new suffixes. histogram descriptions would also need to be updated.
The CQ bit was checked by ryansturm@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...
ryansturm@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: PTAL https://codereview.chromium.org/2477073002/diff/120001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2477073002/diff/120001/components/previews/co... components/previews/core/previews_black_list.cc:9: #include "base/metrics/histogram.h" On 2016/11/09 17:50:51, tbansal1 wrote: > include base/metrics/histogram_macros.h instead of base/metrics/histogram.h? Done. https://codereview.chromium.org/2477073002/diff/120001/components/previews/co... components/previews/core/previews_black_list.cc:79: UMA_HISTOGRAM_BOOLEAN("Previews.OptOut.UserOptedOut.Offline", opt_out); On 2016/11/09 17:50:51, tbansal1 wrote: > May be add a test for this? Done. https://codereview.chromium.org/2477073002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2477073002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48334: +<histogram name="Previews.EligibilityReason.Offline" On 2016/11/09 17:50:52, tbansal1 wrote: > May be name this "Previews.EligibilityReason" and the one below > "Previews.OptOut.UserOptedOut". Then create "Offline" suffix and apply it to > both histograms. Makes it easier to add new suffixes. histogram descriptions > would also need to be updated. I'd prefer to make the change later as we have more consumers of this. It's straightforward to do.
lgtm % comment https://codereview.chromium.org/2477073002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2477073002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48398: +<histogram name="Previews.OptOut.UserOptedOut.Offline" units="Boolean"> Nit: Add a more custom enum that has descriptive labels.
https://codereview.chromium.org/2477073002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2477073002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48398: +<histogram name="Previews.OptOut.UserOptedOut.Offline" units="Boolean"> On 2016/11/09 18:54:12, Alexei Svitkine (very slow) wrote: > Nit: Add a more custom enum that has descriptive labels. Done.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2477073002/#ps160001 (title: "asvitkine nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Adding UMA to track previews opt outs and blacklist eligibility This CL adds UMA to track the reason that a user was not shown a preview (or if it was allowed) and whether a user opts out when shown a preview. BUG=647717 ========== to ========== Adding UMA to track previews opt outs and blacklist eligibility This CL adds UMA to track the reason that a user was not shown a preview (or if it was allowed) and whether a user opts out when shown a preview. BUG=647717 Committed: https://crrev.com/cc61592651c3018a146387f691bc4cbd60f7807b Cr-Commit-Position: refs/heads/master@{#431020} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/cc61592651c3018a146387f691bc4cbd60f7807b Cr-Commit-Position: refs/heads/master@{#431020}
Message was sent while issue was closed.
Looks like this added a flaky test. See crbug.com/663940 for the failures, including https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2... I'm reverting this (by hand) in https://codereview.chromium.org/2491803002/ . Sorry!
Message was sent while issue was closed.
Description was changed from ========== Adding UMA to track previews opt outs and blacklist eligibility This CL adds UMA to track the reason that a user was not shown a preview (or if it was allowed) and whether a user opts out when shown a preview. BUG=647717 Committed: https://crrev.com/cc61592651c3018a146387f691bc4cbd60f7807b Cr-Commit-Position: refs/heads/master@{#431020} ========== to ========== Adding UMA to track previews opt outs and blacklist eligibility This CL adds UMA to track the reason that a user was not shown a preview (or if it was allowed) and whether a user opts out when shown a preview. BUG=647717 Committed: https://crrev.com/cc61592651c3018a146387f691bc4cbd60f7807b Cr-Commit-Position: refs/heads/master@{#431020} ==========
The CQ bit was checked by ryansturm@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 ryansturm@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 ryansturm@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: 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...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by ryansturm@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.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2477073002/#ps220001 (title: "typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adding UMA to track previews opt outs and blacklist eligibility This CL adds UMA to track the reason that a user was not shown a preview (or if it was allowed) and whether a user opts out when shown a preview. BUG=647717 Committed: https://crrev.com/cc61592651c3018a146387f691bc4cbd60f7807b Cr-Commit-Position: refs/heads/master@{#431020} ========== to ========== Adding UMA to track previews opt outs and blacklist eligibility This CL adds UMA to track the reason that a user was not shown a preview (or if it was allowed) and whether a user opts out when shown a preview. BUG=647717 Committed: https://crrev.com/cc61592651c3018a146387f691bc4cbd60f7807b Cr-Commit-Position: refs/heads/master@{#431020} ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Adding UMA to track previews opt outs and blacklist eligibility This CL adds UMA to track the reason that a user was not shown a preview (or if it was allowed) and whether a user opts out when shown a preview. BUG=647717 Committed: https://crrev.com/cc61592651c3018a146387f691bc4cbd60f7807b Cr-Commit-Position: refs/heads/master@{#431020} ========== to ========== Adding UMA to track previews opt outs and blacklist eligibility This CL adds UMA to track the reason that a user was not shown a preview (or if it was allowed) and whether a user opts out when shown a preview. BUG=647717 Committed: https://crrev.com/cc61592651c3018a146387f691bc4cbd60f7807b Committed: https://crrev.com/af5258ab4404766182e87f2c773051684c7d8c72 Cr-Original-Commit-Position: refs/heads/master@{#431020} Cr-Commit-Position: refs/heads/master@{#431181} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/af5258ab4404766182e87f2c773051684c7d8c72 Cr-Commit-Position: refs/heads/master@{#431181} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
