Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2671983002:
ntp_tiles: Add UMA histograms to correlate icon type to position (Closed)
Descriptionntp_tiles: Add UMA histograms to correlate icon type to position
The previously instrumented UMA histograms measure the number of tiles
per icon type, but not their position. We suspect there could be a
correlation between the position and the type, so new histograms are
proposed to verify this.
BUG=688365
Review-Url: https://codereview.chromium.org/2671983002
Cr-Commit-Position: refs/heads/master@{#449316}
Committed: https://chromium.googlesource.com/chromium/src/+/8619a463c1a4ec861c542c9482c502a6df96acdf
Patch Set 1 #Patch Set 2 : Fixes. #Patch Set 3 : Fixes. #
Total comments: 8
Patch Set 4 : Fixes. #
Total comments: 4
Patch Set 5 : Addressed comments. #
Total comments: 3
Patch Set 6 : Adopted histogram suffixes. #
Messages
Total messages: 23 (10 generated)
mastiz@chromium.org changed reviewers: + treib@chromium.org
Perhaps I should deprecate the previously existing UMA metrics, but that'd also make hard to compare versions.
Something seems to have gone wrong with the histograms.xml upload; I can't see the diff..? https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... File components/ntp_tiles/metrics.cc (right): https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... components/ntp_tiles/metrics.cc:29: // Prefixes for the various icon types. nit: Suffixes https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... components/ntp_tiles/metrics.cc:123: "NewTabPage.SuggestionsImpressionIcon.%s", icon_type_suffix); Hrm, somehow all our metrics use different naming conventions (here "Icon.Color", below ".IconsColor"). But I like this naming scheme more than the below one, so...eh. https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... components/ntp_tiles/metrics.cc:150: "NewTabPage.MostVisitedClickIcon.%s", icon_type_suffix); Just MostVisitedIcon, to be slightly more consistent with the other MostVisited histograms?
PTAL. FYI, I'm getting the following error which might explain the lack of diff for histograms.xml: "Not uploading the base file for tools/metrics/histograms/histograms.xml because it's too large." https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... File components/ntp_tiles/metrics.cc (right): https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... components/ntp_tiles/metrics.cc:29: // Prefixes for the various icon types. On 2017/02/03 15:03:31, Marc Treib wrote: > nit: Suffixes Done. https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... components/ntp_tiles/metrics.cc:123: "NewTabPage.SuggestionsImpressionIcon.%s", icon_type_suffix); On 2017/02/03 15:03:31, Marc Treib wrote: > Hrm, somehow all our metrics use different naming conventions (here > "Icon.Color", below ".IconsColor"). But I like this naming scheme more than the > below one, so...eh. Acknowledged. https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... components/ntp_tiles/metrics.cc:150: "NewTabPage.MostVisitedClickIcon.%s", icon_type_suffix); On 2017/02/03 15:03:31, Marc Treib wrote: > Just MostVisitedIcon, to be slightly more consistent with the other MostVisited > histograms? Unless you feel strongly, I'd really like to put Click on the name. The old histogram is very badly named and can be mistakenly interpreted as impression (this one about icons even more).
On 2017/02/06 10:59:49, mastiz wrote: > PTAL. > > FYI, I'm getting the following error which might explain the lack of diff for > histograms.xml: > > "Not uploading the base file for tools/metrics/histograms/histograms.xml because > it's too large." Nah, that's normal. In the latest patch set I can see the diff, so all good. I'm looking at the code now.
On 2017/02/06 10:59:49, mastiz wrote: > PTAL. > > FYI, I'm getting the following error which might explain the lack of diff for > histograms.xml: > > "Not uploading the base file for tools/metrics/histograms/histograms.xml because > it's too large." Nah, that's normal. In the latest patch set I can see the diff, so all good. I'm looking at the code now.
lgtm with nits https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... File components/ntp_tiles/metrics.cc (right): https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... components/ntp_tiles/metrics.cc:150: "NewTabPage.MostVisitedClickIcon.%s", icon_type_suffix); On 2017/02/06 10:59:49, mastiz wrote: > On 2017/02/03 15:03:31, Marc Treib wrote: > > Just MostVisitedIcon, to be slightly more consistent with the other > MostVisited > > histograms? > > Unless you feel strongly, I'd really like to put Click on the name. The old > histogram is very badly named and can be mistakenly interpreted as impression > (this one about icons even more). Eh.. I tend to value consistency over accuracy, but here I don't feel strongly enough to argue. https://codereview.chromium.org/2671983002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2671983002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39788: Histogram for user clicks of the most visited thumbnails. The value is equal Also here s/thumbnails/tiles/ ? https://codereview.chromium.org/2671983002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2671983002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39811: + Histogram for clicks on the various most visited tiles using a fallback Why "various"? nit: I'd remove the "Histogram for" preamble; that's obvious.
https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... File components/ntp_tiles/metrics.cc (right): https://codereview.chromium.org/2671983002/diff/40001/components/ntp_tiles/me... components/ntp_tiles/metrics.cc:150: "NewTabPage.MostVisitedClickIcon.%s", icon_type_suffix); On 2017/02/06 11:09:54, Marc Treib wrote: > On 2017/02/06 10:59:49, mastiz wrote: > > On 2017/02/03 15:03:31, Marc Treib wrote: > > > Just MostVisitedIcon, to be slightly more consistent with the other > > MostVisited > > > histograms? > > > > Unless you feel strongly, I'd really like to put Click on the name. The old > > histogram is very badly named and can be mistakenly interpreted as impression > > (this one about icons even more). > > Eh.. I tend to value consistency over accuracy, but here I don't feel strongly > enough to argue. Thanks, I'll then stick to having Clicks in the name. https://codereview.chromium.org/2671983002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2671983002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39788: Histogram for user clicks of the most visited thumbnails. The value is equal On 2017/02/06 11:09:54, Marc Treib wrote: > Also here s/thumbnails/tiles/ ? Done, thx. https://codereview.chromium.org/2671983002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2671983002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39811: + Histogram for clicks on the various most visited tiles using a fallback On 2017/02/06 11:09:54, Marc Treib wrote: > Why "various"? > > nit: I'd remove the "Histogram for" preamble; that's obvious. Done.
mastiz@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@: please take a look at histograms.xml, thx.
https://codereview.chromium.org/2671983002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2671983002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39837: +<histogram name="NewTabPage.MostVisitedClickIcon.Real" Please use histogram_suffixes construct to void duplication here.
The CQ bit was checked by mastiz@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...
PTAL. https://codereview.chromium.org/2671983002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2671983002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39837: +<histogram name="NewTabPage.MostVisitedClickIcon.Real" On 2017/02/07 16:03:17, Alexei Svitkine (slow) wrote: > Please use histogram_suffixes construct to void duplication here. Done. This made me thing whether we should actually use the already existing Histograms (SuggestionsImpression, MostVisisted), since the ones introduced here are breakdowns per icon appearance. However, another breakdown/suffix already exists for those (for the actual data-source that provides the suggestion), which could potentially create (a) confusion and (b) naming collisions. Nevertheless, I went ahead with this idea. Is this a common way to express multiple breakdowns per metric?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2671983002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2671983002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39837: +<histogram name="NewTabPage.MostVisitedClickIcon.Real" On 2017/02/09 08:23:37, mastiz wrote: > On 2017/02/07 16:03:17, Alexei Svitkine (slow) wrote: > > Please use histogram_suffixes construct to void duplication here. > > Done. > > This made me thing whether we should actually use the already existing > Histograms (SuggestionsImpression, MostVisisted), since the ones introduced here > are breakdowns per icon appearance. However, another breakdown/suffix already > exists for those (for the actual data-source that provides the suggestion), > which could potentially create (a) confusion and (b) naming collisions. > > Nevertheless, I went ahead with this idea. Is this a common way to express > multiple breakdowns per metric? Yep, this is the suggested way when doing this sort of thing.
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2671983002/#ps100001 (title: "Adopted histogram suffixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486658344998910,
"parent_rev": "d6076bd93b8d7171ac7605bc91e835b0c2965a55", "commit_rev":
"8619a463c1a4ec861c542c9482c502a6df96acdf"}
Message was sent while issue was closed.
Description was changed from ========== ntp_tiles: Add UMA histograms to correlate icon type to position The previously instrumented UMA histograms measure the number of tiles per icon type, but not their position. We suspect there could be a correlation between the position and the type, so new histograms are proposed to verify this. BUG=688365 ========== to ========== ntp_tiles: Add UMA histograms to correlate icon type to position The previously instrumented UMA histograms measure the number of tiles per icon type, but not their position. We suspect there could be a correlation between the position and the type, so new histograms are proposed to verify this. BUG=688365 Review-Url: https://codereview.chromium.org/2671983002 Cr-Commit-Position: refs/heads/master@{#449316} Committed: https://chromium.googlesource.com/chromium/src/+/8619a463c1a4ec861c542c9482c5... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/8619a463c1a4ec861c542c9482c5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
