Desktop NTP thumbnail metrics contribute to wrong UMA histogram buckets |
|||||||||
Issue descriptionOriginally reported by rpop@: analyzing UMA data for dev surfaces the fact that UMA histogram NewTabPage.TileType look very fishy. The wrong buckets are populated for desktop tiles, both for impressions and clicks. The metrics were introduced in M59, by https://codereview.chromium.org/2796643002.
,
Apr 25 2017
If this lands before M59 is released to Beta, then a merge might still be plausible.
,
Apr 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1 commit 79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1 Author: mastiz <mastiz@chromium.org> Date: Wed Apr 26 10:30:26 2017 NTP: Fix TileType metrics on desktop Follow-up to https://codereview.chromium.org/2796643002 where the metrics were added but an inconsistency was introduced between the C++ enum and the Javascript counterpart. This caused the wrong buckets being counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail; ThumbnailServer instead of ThumbnailFailed). The affected buckets were already deprecated (DeprecatedThumbnailLocal and DeprecatedThumbnailServer) and hence, luckily, there are no conflated buckets. Besides, histograms.xml was missing the new suffixes for NewTabPageIconTypes, used to provide desktop-specific breakdowns for NewTabPage.MostVisited and NewTabPage.SuggestionsImpression. BUG= 714095 , 703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2832173002 Cr-Commit-Position: refs/heads/master@{#467287} [modify] https://crrev.com/79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1/chrome/browser/resources/local_ntp/most_visited_single.js [modify] https://crrev.com/79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1/tools/metrics/histograms/histograms.xml
,
Apr 26 2017
Two of our team members carefully tested this change which affects UMA-related code only and is rather trivial, so I'm going ahead with a merge request.
,
Apr 26 2017
,
Apr 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ccbb674bcfe3f1f1395235edd0bb433670a63837 commit ccbb674bcfe3f1f1395235edd0bb433670a63837 Author: Mikel Astiz <mastiz@chromium.org> Date: Thu Apr 27 08:40:54 2017 NTP: Fix TileType metrics on desktop Follow-up to https://codereview.chromium.org/2796643002 where the metrics were added but an inconsistency was introduced between the C++ enum and the Javascript counterpart. This caused the wrong buckets being counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail; ThumbnailServer instead of ThumbnailFailed). The affected buckets were already deprecated (DeprecatedThumbnailLocal and DeprecatedThumbnailServer) and hence, luckily, there are no conflated buckets. Besides, histograms.xml was missing the new suffixes for NewTabPageIconTypes, used to provide desktop-specific breakdowns for NewTabPage.MostVisited and NewTabPage.SuggestionsImpression. BUG= 714095 , 703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2832173002 Cr-Commit-Position: refs/heads/master@{#467287} (cherry picked from commit 79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1) Review-Url: https://codereview.chromium.org/2845863003 . Cr-Commit-Position: refs/branch-heads/3071@{#261} Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641} [modify] https://crrev.com/ccbb674bcfe3f1f1395235edd0bb433670a63837/chrome/browser/resources/local_ntp/most_visited_single.js [modify] https://crrev.com/ccbb674bcfe3f1f1395235edd0bb433670a63837/tools/metrics/histograms/histograms.xml
,
Apr 28 2017
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 1 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 2 2017
This has already been merged, so removing the merge-approved label.
,
May 3 2017
Thanks marc. Marking this as fixed although 59.0.3071.30 or above is not yet on beta. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by mastiz@chromium.org
, Apr 21 2017