@newt: can you sanity check Patrick's proposal?
I'll look into preparing a fix later today.

can you clarify which precise problem you're trying to solve? i.e., what question are you trying to answer with that histogram?

My suspicion is that this metric got "broken" when the bookmarks link was removed from the NTP (--> move to material design).

It's a bit unclear to me however, if the metrics are wrong or simply deprecated. Note, that they still get logged through "Stars.LaunchLocation" which is probably sufficient for the bookmarks case.

For the current Zine integration, we record bookmark interactions through a completely different path. They contribute to NewTabPage.ActionAndroid with action type "ACTION_OPENED_SNIPPET" -- i.e., they get handled like any other content suggestion.
If you're interested in a breakdown per category, NewTabPage.ContentSuggestions.Opened.Bookmarks is what you're looking for.

Hi Tim,

the ActionAndroid histogram is supposed to measure "Actions taken by users from the new tab page on Android. These actions may navigate away from the NTP (e.g. searching in the omnibox or opening a bookmark), but can also happen without navigating away from the NTP (e.g. opening a bookmark in a new tab)."

Without this histogram, we cannot reason about the volume of interactions with the NTP and how users use the NTP in general. We've been using this histogram to determine if any of our changes to the above or below the fold section increase the number of users who interact with the NTP, and whether it increase the number of interactions per user.

What I mentioned above is that it seems like the histogram is now spoiled with all bookmark visits from the Bookmark Manager (instead of just those coming from the NTP through the overflow menu). Can you confirm that this only broke recently? Otherwise we will need to re-visit our metrics for earlier launches as well.

I strongly suggest to fix this metric asap, because otherwise we will always need to piece together interaction metrics for the NTP from various sources, which makes using the Finch dashboard and comparing experiment groups much harder to do.

So, my specific request would be: Let's make sure that NewTabPageUma.ACTION_OPENED_BOOKMARK only measures bookmarks opened from the NTP, i.e. through the overflow menu on the NTP or via Recent bookmarks.

And if possible, I would like to merge this to M55.

As far as I can tell, this has been the case for a long time, at least since https://codereview.chromium.org/1475513008, 11 months ago.

I guess the justification might have been that the bookmark manager was considered more or less a part of the NTP?

Anyway, can't we just ignore the "opened a bookmark" bucket in ActionAndroid for our purposes? It shouldn't be possible to open a bookmark from the Zine NTP (except via the Bookmarks section, which is tracked differently).

(All that being said: The ActionAndroid histogram is clearly broken in this regard and should be fixed, but I don't think it's super urgent or even merge-critical for 55.)

+fgorski

This means this metrics has been broken since M49.

It would still be good to know what the exit paths from the NTP are (including the overflow menu), so maybe we should do two things:

1) remove tracking of Bookmark Manager clicks from ActionAndroid
2) add new UMA metrics to track interactions with the overflow menu specifically on the NTP

Looks like I dropped the ball on this one. I agree that I caused the metric to inflate. I recall being puzzled about the name of a metric that counts how many times the bookmark is opened (as that part was unconditional). I should have reached out to NTP.

On the other, it was reported unconditionally when calling (Enhanced)BoomarkManager#openBookmark to open a bookmark, and I don't see this call being specific to NTP only. Therefore it is possible that the metric was already inflated before my change. I think in the light of that starting fresh would make sense.

Somehow some of bookmarks' metrics are named after NTP. This might came from the fact that bookmark manager used to be part of the NTP? Anyway, here is a complete list of bookmark related metrics:

Opening a bookmark from the manager: MobileNTPBookmark
Opening the manager from the menu: MobileMenuAllBookmarks
Opening the manager from the access point on the NTP (removed with Zine): MobileNTPSwitchToBookmarks
Saved a bookmark (by clicking the star icon in the menu): MobileMenuAddToBookmarks


I think what nepper@ initially proposed makes sense. NewTabPage.ActionAndroid should not include MobileNTPBookmark. Yet bookmark manager still should use MobileNTPBookmark separately on its own purpose.

Would it be feasible to rename the metrics to avoid further confusion? I guess renaming would mean replace with a new one. Not sure about the implications for your analysis though. If applicable, any proposals for a new name? 

I'd also propose to delete MobileNTPSwitchToBookmarks

Yes this is feasible.

MobileBookmarkOpen sounds like a good name to me.

I started fixing the issue Patrick mentioned in comment #1 (https://codereview.chromium.org/2528303003).
But I just realized that we might have the same problem with recent tabs.

M56 Beta is around the corner. How are we doing here?

Friendly ping :)

3 weeks ago https://chromiumcodereview.appspot.com/2504293004/ was created, yet thus far no patch has been updated.

I've now send out https://codereview.chromium.org/2528303003 for review. 
It now works as follows (on Android!):

if (snippets == disabled)
  - click on NTP bookmarks entry point -> NewTabPageUma.ACTION_OPENED_BOOKMARK
  - click on NTP recent tabs entry point -> NewTabPageUma.ACTION_OPENED_RECENTLY_CLOSED_ENTRY

if (snippets == enabled)
  - click on MORE button in bookmarks section -> NewTabPageUma.ACTION_OPENED_BOOKMARK
  - click on MORE button in recent tabs -> NewTabPageUma.ACTION_OPENED_RECENTLY_CLOSED_ENTRY

if (current page == NTP)
  - click on overflow bookmarks -> NewTabPageUma.ACTION_OPENED_BOOKMARK
  - click on overflow recent tabs -> NewTabPageUma.ACTION_OPENED_RECENTLY_CLOSED_ENTRY

if (current page == recent tabs view)
 - click on recent tab -> UserAction.MobileRecentlyClosedTabOpen
 - click on foreign session ->  UserAction.MobileForeignSessionOpen

if (current page == bookmarks view)
 - click on bookmarks -> UserAction.MobileBookmarkOpen


Things left to do:
- renaming the constants a bit (current names can be a bit misleading)
- also fix it on iOS

Any comments?

iOS is upstreamed now, can't we update it at the same time? The change above moves things around but does not seem to make things right still. The metrics after that has landed will still be not very comprehensible because the names and descriptions don't reflect what the actions track. Which is why I'd prefer to fix the whole thing at once, provided it doesn't result in a too big CL (seeing the size of the current one I think it will be ok.)

Making the change in one go is certainly better. But then someone else has to do it. I'm not familiar with our iOS code base :-)

I've now replaced NewTabPage.ActionAndroid with NewTabPage.ActionAndroid2. It now has the following buckets:

ACTION_SEARCHED_USING_OMNIBOX
ACTION_NAVIGATED_TO_GOOGLE_HOMEPAGE
ACTION_NAVIGATED_USING_OMNIBOX
ACTION_OPENED_MOST_VISITED_TILE
ACTION_OPENED_RECENT_TABS_MANAGER -> new: user opened the recent tabs manager via the more button *or* the overflow menu
ACTION_OPENED_HISTORY_MANAGER -> new: user opened the history manager via the overflow menu
ACTION_OPENED_BOOKMARKS_MANAGER -> new: user opened the bookmarks manager via the more button *or* the overflow menu
ACTION_OPENED_DOWNLOADS_MANAGER -> new: user opened the downloads manager via the more button *or* the overflow menu
ACTION_OPENED_SNIPPET
ACTION_CLICKED_LEARN_MORE
ACTION_CLICKED_ALL_DISMISSED_REFRESH

This should give us a complete overview of how the user interacted with the NTP.

I also renamed the following user actions to make clear that they are not related to the NTP:

MobileNTPBookmark -> MobileBookmarkManagerEntryOpened
MobileNTPForeignSession -> MobileRecentTabManagerRecentTabOpened
MobileNTPRecentlyClosed -> MobileRecentTabManagerRecentTabOpened

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

commit 422ecccc1f41cb2c5ec5632727a8265b40a5d1d2
Author: finkm <finkm@chromium.org>
Date: Tue Jan 10 16:12:58 2017

- deprecated histogram NewTabPage.ActionAndroid and replaced it with
  NewTabPage.ActionAndroid2
- replaced action MobileNTPBookmark with MobileBookmarkManagerEntryOpened
- replaced action MobileNTPForeignSession with MobileRecentTabManagerTabFromOtherDeviceOpened
- replaced action MobileNTPRecentlyClosed with MobileRecentTabManagerRecentTabOpened

For details please refer to the linked bug.

BUG= 662327 

Review-Url: https://codereview.chromium.org/2528303003
Cr-Commit-Position: refs/heads/master@{#442599}

[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsManager.java
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/ios/chrome/browser/metrics/first_user_action_recorder.cc
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/ios/chrome/browser/ui/bookmarks/bookmark_interaction_controller.mm
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/ios/chrome/browser/ui/ntp/recent_tabs/recent_tabs_table_view_controller.mm
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/tools/metrics/actions/actions.xml
[modify] https://crrev.com/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2/tools/metrics/histograms/histograms.xml

Woot! Did you get a chance to confirm this on Canary or Dev already so that we can merge this back to the release branch? :)