Yup, the current metrics setup unfortunately doesn't work well for PopularSites. :(

I assume the same problem applies to iOS.

is there a way we can mark this graph as broken in the UMA dashboards?
I'm afraid somebody tries to draw conclusions from that data...

I don't think there's a canonical way to do that. We could add something to the description text.

Increasing prio because:
 - iOS currently reports no icon types (they resolve icons only when shown, the ntp_tiles::metrics function reports it all per NTP impression, iOS thus knows no types at that moment).
 - Since M58 (probably due to changes by I/O Scheduler), the type NONE is overrepresented also for TileType.server. We simply sometimes report too early, the reporting is fragile.

This makes it hard / impossible to evaluate the experiment to fetch favicons for .server tiles.

Proposed solution:
Split ntp_tiles::metrics::RecordPageImpression() into:
 - RecordPageImpression (called as now)
 - RecordTileImpression (called whenever a tile is first impressed on an NTP, after its favicon type is resolved)

Grabbing this for ntp_tiles and for Android.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/437702b594f4bb8d95088af5bc05c4cd82d4dfc1

commit 437702b594f4bb8d95088af5bc05c4cd82d4dfc1
Author: Jan Krcal <jkrcal@chromium.org>
Date: Fri Jun 23 09:20:48 2017

[NTP Tiles metrics] Separate page impression for tile impressions

On Android, we currently report many tiles of type NONE. On iOS we even
do not report tiles at all because the type is unknown.

This CL allows to delay the reporting of tile impressions until we get
result from FaviconService. The actual delay will be implemented in
further CLs.

Bug:  695806 
Change-Id: Ia1fba47b270e42d597edab3f82bfa25b40cec37c
Reviewed-on: https://chromium-review.googlesource.com/542856
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Michael van Ouwerkerk <mvanouwerkerk@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481833}
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSites.java
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/FakeMostVisitedSites.java
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/chrome/browser/android/ntp/most_visited_sites_bridge.cc
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/chrome/browser/android/ntp/most_visited_sites_bridge.h
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/chrome/browser/ui/search/ntp_user_data_logger.cc
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/components/ntp_tiles/metrics.cc
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/components/ntp_tiles/metrics.h
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/components/ntp_tiles/metrics_unittest.cc
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/ios/chrome/browser/content_suggestions/mediator_util.h
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/ios/chrome/browser/content_suggestions/mediator_util.mm
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/ios/chrome/browser/ui/ntp/google_landing_mediator.mm
[modify] https://crrev.com/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/tools/metrics/histograms/histograms.xml

Gauthier, can you please take it over for iOS?
(the CL from #6 did not change anything there)

Nicolas, can you please take it over for current NTP on Android?
(as discussed offline)

Sure.
Should I remove the ntp_tiles::metrics::RecordTileImpression you added with a UNKNOW_TILE_TYPE (https://chromium.googlesource.com/chromium/src/+/437702b594f4bb8d95088af5bc05c4cd82d4dfc1/ios/chrome/browser/content_suggestions/mediator_util.mm#130), or do we expect two recording: one unknown when the tile is first displayed (before fetching) and one after the favicon is fetched?
From #4, I would expect only one recording, after fetching the favicon.

Correct, only one recording, after fetching the favicon.

Thanks!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f68afc44b3f773ed6cf44d4c7e5c9438dadb07c

commit 4f68afc44b3f773ed6cf44d4c7e5c9438dadb07c
Author: gambard <gambard@chromium.org>
Date: Thu Jun 29 16:20:38 2017

Log the most visited tiles impression

This CL logs the Most Visited tiles impression after receiving the
favicon and determining the tile type.
Only the favicon displayed are logged (i.e. if only 6 tiles are shown
because the screen width is not large enough, only 6 impressions are
logged).

BUG= 695806 

Change-Id: Iea95b44bea030b60733d6f11581ec2fa1b0df448
Reviewed-on: https://chromium-review.googlesource.com/549364
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483379}
[modify] https://crrev.com/4f68afc44b3f773ed6cf44d4c7e5c9438dadb07c/ios/chrome/browser/ui/ntp/google_landing_mediator.mm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5efff19d2f63af0e6c3721a57c2abae661df1c2d

commit 5efff19d2f63af0e6c3721a57c2abae661df1c2d
Author: Nicolas Dossou-gbete <dgn@chromium.org>
Date: Fri Jun 30 12:46:02 2017

[Suggestions] Wait for complete tile load before recording the results

We used to mark the tile load task as completed as soon as we got
results from the native code, but before processing the results
and updating the tile data. This resulted in logging incomplete
data about the tiles.

Bug:  695806 
Change-Id: Idc65803288f078263ad08d4105e92da1face58b3
Reviewed-on: https://chromium-review.googlesource.com/545897
Commit-Queue: Nicolas Dossou-Gbété <dgn@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Michael van Ouwerkerk <mvanouwerkerk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483686}
[modify] https://crrev.com/5efff19d2f63af0e6c3721a57c2abae661df1c2d/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
[modify] https://crrev.com/5efff19d2f63af0e6c3721a57c2abae661df1c2d/chrome/android/java_sources.gni
[add] https://crrev.com/5efff19d2f63af0e6c3721a57c2abae661df1c2d/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageLoadTest.java

This should be fixed for iOS for the current NTP. I keep the bug open because I need to do it for the new implementation which should ship in M62.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/10ffa9606e2d27809435f3bc59c5c94bc81cef6c

commit 10ffa9606e2d27809435f3bc59c5c94bc81cef6c
Author: gambard <gambard@chromium.org>
Date: Wed Jul 05 07:57:35 2017

Record MostVisited tile impression type

This CL records the impression of the most visited tiles.
The recording is done individually, once the favicon is fetched.

BUG= 695806 

Change-Id: Ic511a57b91ae3bacdfa3716715e170164debf760
Reviewed-on: https://chromium-review.googlesource.com/559003
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Elodie Banel <lod@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484209}
[modify] https://crrev.com/10ffa9606e2d27809435f3bc59c5c94bc81cef6c/ios/chrome/browser/content_suggestions/content_suggestions_favicon_mediator.h
[modify] https://crrev.com/10ffa9606e2d27809435f3bc59c5c94bc81cef6c/ios/chrome/browser/content_suggestions/content_suggestions_favicon_mediator.mm
[modify] https://crrev.com/10ffa9606e2d27809435f3bc59c5c94bc81cef6c/ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
[modify] https://crrev.com/10ffa9606e2d27809435f3bc59c5c94bc81cef6c/ios/chrome/browser/content_suggestions/mediator_util.h
[modify] https://crrev.com/10ffa9606e2d27809435f3bc59c5c94bc81cef6c/ios/chrome/browser/content_suggestions/mediator_util.mm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b7d5da805df04dcc4b12c5e2a5188491e02f7457

commit b7d5da805df04dcc4b12c5e2a5188491e02f7457
Author: gambard <gambard@chromium.org>
Date: Thu Jul 06 08:49:46 2017

Fix logging removed by ARC conversion

The function call for logging the tile impression has been removed
during ARC conversion. This CL adds it back.
Removed in https://codereview.chromium.org/2955363002

BUG= 695806 

Change-Id: I9460c9cffb89c924be352204e7ea3daeb330e8ad
Reviewed-on: https://chromium-review.googlesource.com/559090
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484502}
[modify] https://crrev.com/b7d5da805df04dcc4b12c5e2a5188491e02f7457/ios/chrome/browser/ui/ntp/google_landing_mediator.mm

It is fixed for iOS. From my understanding, it is also fixed for Android.
dgn@, please re-open if this is not the case.