New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 714095 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 703165



Sign in to add a comment

Desktop NTP thumbnail metrics contribute to wrong UMA histogram buckets

Project Member Reported by mastiz@chromium.org, Apr 21 2017

Issue description

Originally 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.
 

Comment 1 by mastiz@chromium.org, Apr 21 2017

Blocking: 703165

Comment 2 by treib@chromium.org, Apr 25 2017

Labels: M-59
If this lands before M59 is released to Beta, then a merge might still be plausible.
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Comment 4 by mastiz@chromium.org, Apr 26 2017

Labels: Merge-Request-59
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.

Comment 5 by gkihumba@google.com, Apr 26 2017

Labels: Merge-Approved-59
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 27 2017

Labels: -merge-approved-59 merge-merged-3071
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

Project Member

Comment 7 by sheriffbot@chromium.org, Apr 28 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
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
Project Member

Comment 8 by sheriffbot@chromium.org, May 1 2017

Cc: gkihumba@google.com
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

Comment 9 by treib@chromium.org, May 2 2017

Labels: -Merge-Approved-59
This has already been merged, so removing the merge-approved label.
Status: Fixed (was: Assigned)
Thanks marc. Marking this as fixed although 59.0.3071.30 or above is not yet on beta.

Sign in to add a comment