[Remote suggestions] Measure separate data use for JSONs and thumbnails

[Remote suggestions] Measure separate data use for JSONs and thumbnails

Before this CL, these two cases are measured in one DataUseUserData
enum element. This CL adds another enum element and renames the
previous one for better clarity.

It is important to split the measurements in order to get clearer insights because there are of very different nature: 
 - The JSONs are background data that is used for every user (to some extent) independently of usage of Chrome. 
 - On the other hand, the thumbnails are loaded on demand only when the feature is used.

BUG=688945
Review-Url: https://codereview.chromium.org/2685523002
Cr-Commit-Position: refs/heads/master@{#449563}
Committed: https://chromium.googlesource.com/chromium/src/+/cd18da6b82c146850fc2a10700ae89e1f21e329a

[jkrcal, 2017-02-07 11:10:26]

jkrcal@chromium.org changed reviewers:
+ bengr@chromium.org, treib@chromium.org

[jkrcal, 2017-02-07 11:10:26]

Marc, could you PTAL?

Ben, could you PTAL at components/data_use_measurement/*?
Robert, could you PTAL at histograms.xml?

[jkrcal, 2017-02-07 11:14:53]

Description was changed from

==========
[Remote suggestions] Measure separate data use for JSONs and thumbnails

Before this CL, these two cases are measured in one DataUseUserData
enum element. This CL adds another enum element and renames the
previous one for better clarity.

BUG=688945
==========

to

==========
[Remote suggestions] Measure separate data use for JSONs and thumbnails

Before this CL, these two cases are measured in one DataUseUserData
enum element. This CL adds another enum element and renames the
previous one for better clarity.

It is important to split the measurements in order to get clearer insights
because there are of very different nature: 
 - The JSONs are background data that is used for every user (to some extent)
independently of usage of Chrome. 
 - On the other hand, the thumbnails are loaded on demand only when the feature
is used.

BUG=688945
==========

[jkrcal, 2017-02-07 11:14:53]

jkrcal@chromium.org changed reviewers:
+ rkaplow@chromium.org

[jkrcal, 2017-02-07 11:15:42]

I've forgotten to add Robert to reviews' list:

Robert, could you PTAL at histograms.xml?

[Marc Treib, 2017-02-07 11:20:48]

lgtm

https://codereview.chromium.org/2685523002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/2685523002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_user_data.h:42:
NTP_SNIPPETS_SUGGESTIONS,
This means the existing tag will change its definition. I guess we're okay with
that?
The alternative would be to deprecate this tag and introduce a new one for
suggestions. But AFAIK there's no way to deprecate only a single enum entry in
histograms.xml, so that would probably introduce even more confusion.

[jkrcal, 2017-02-07 13:38:57]

Thanks!

https://codereview.chromium.org/2685523002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/2685523002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_user_data.h:42:
NTP_SNIPPETS_SUGGESTIONS,
On 2017/02/07 11:20:47, Marc Treib wrote:
> This means the existing tag will change its definition. I guess we're okay
with
> that?
> The alternative would be to deprecate this tag and introduce a new one for
> suggestions. But AFAIK there's no way to deprecate only a single enum entry in
> histograms.xml, so that would probably introduce even more confusion.

Personally, I am fine with that. 
Maybe adding a comment to the code to document the change?

[Marc Treib, 2017-02-07 13:44:52]

https://codereview.chromium.org/2685523002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/2685523002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_user_data.h:42:
NTP_SNIPPETS_SUGGESTIONS,
On 2017/02/07 13:38:57, jkrcal wrote:
> On 2017/02/07 11:20:47, Marc Treib wrote:
> > This means the existing tag will change its definition. I guess we're okay
> with
> > that?
> > The alternative would be to deprecate this tag and introduce a new one for
> > suggestions. But AFAIK there's no way to deprecate only a single enum entry
in
> > histograms.xml, so that would probably introduce even more confusion.
> 
> Personally, I am fine with that. 
> Maybe adding a comment to the code to document the change?

A comment in the histogram description might make sense, but since this is just
an enum in histograms.xml, I don't think that's feasible.
Anyway, I also think this is okay. Just checking.

[rkaplow, 2017-02-07 15:41:22]

https://codereview.chromium.org/2685523002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/2685523002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_user_data.h:42:
NTP_SNIPPETS_SUGGESTIONS,
On 2017/02/07 13:44:51, Marc Treib wrote:
> On 2017/02/07 13:38:57, jkrcal wrote:
> > On 2017/02/07 11:20:47, Marc Treib wrote:
> > > This means the existing tag will change its definition. I guess we're okay
> > with
> > > that?
> > > The alternative would be to deprecate this tag and introduce a new one for
> > > suggestions. But AFAIK there's no way to deprecate only a single enum
entry
> in
> > > histograms.xml, so that would probably introduce even more confusion.
> > 
> > Personally, I am fine with that. 
> > Maybe adding a comment to the code to document the change?
> 
> A comment in the histogram description might make sense, but since this is
just
> an enum in histograms.xml, I don't think that's feasible.
> Anyway, I also think this is okay. Just checking.

actually deprecating an enum is fine - wejust suggest appending (obsolete) to
the enum label, and eventually the traffic to it will go away. 

This way you will get data mixed together from the 2 different versions, so
unless you're prepared for that to happen we generally suggest a new enum

[jkrcal, 2017-02-07 17:25:44]

Thanks, PTAL again!

(we would also be fine with mixing the data but deprecating it is simple enough)

https://codereview.chromium.org/2685523002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/2685523002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_user_data.h:42:
NTP_SNIPPETS_SUGGESTIONS,
On 2017/02/07 15:41:21, rkaplow wrote:
> On 2017/02/07 13:44:51, Marc Treib wrote:
> > On 2017/02/07 13:38:57, jkrcal wrote:
> > > On 2017/02/07 11:20:47, Marc Treib wrote:
> > > > This means the existing tag will change its definition. I guess we're
okay
> > > with
> > > > that?
> > > > The alternative would be to deprecate this tag and introduce a new one
for
> > > > suggestions. But AFAIK there's no way to deprecate only a single enum
> entry
> > in
> > > > histograms.xml, so that would probably introduce even more confusion.
> > > 
> > > Personally, I am fine with that. 
> > > Maybe adding a comment to the code to document the change?
> > 
> > A comment in the histogram description might make sense, but since this is
> just
> > an enum in histograms.xml, I don't think that's feasible.
> > Anyway, I also think this is okay. Just checking.
> 
> actually deprecating an enum is fine - wejust suggest appending (obsolete) to
> the enum label, and eventually the traffic to it will go away. 
> 
> This way you will get data mixed together from the 2 different versions, so
> unless you're prepared for that to happen we generally suggest a new enum

Fair enough, done.

[rkaplow, 2017-02-07 17:33:58]

lgtm

[bengr, 2017-02-09 17:34:45]

lgtm

[jkrcal, 2017-02-09 18:41:57]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 18:42:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 21:10:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 21:10:26]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-02-10 07:48:55]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-02-10 07:48:55]

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/2685523002/#ps20001
(title: "Comments #1")

[commit-bot: I haz the power, 2017-02-10 07:49:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 07:53:54]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486712935333680,
"parent_rev": "53e1c483fe44e3544dbd872911c2914777e992e6", "commit_rev":
"cd18da6b82c146850fc2a10700ae89e1f21e329a"}

[commit-bot: I haz the power, 2017-02-10 07:54:31]

Description was changed from

==========
[Remote suggestions] Measure separate data use for JSONs and thumbnails

Before this CL, these two cases are measured in one DataUseUserData
enum element. This CL adds another enum element and renames the
previous one for better clarity.

It is important to split the measurements in order to get clearer insights
because there are of very different nature: 
 - The JSONs are background data that is used for every user (to some extent)
independently of usage of Chrome. 
 - On the other hand, the thumbnails are loaded on demand only when the feature
is used.

BUG=688945
==========

to

==========
[Remote suggestions] Measure separate data use for JSONs and thumbnails

Before this CL, these two cases are measured in one DataUseUserData
enum element. This CL adds another enum element and renames the
previous one for better clarity.

It is important to split the measurements in order to get clearer insights
because there are of very different nature: 
 - The JSONs are background data that is used for every user (to some extent)
independently of usage of Chrome. 
 - On the other hand, the thumbnails are loaded on demand only when the feature
is used.

BUG=688945

Review-Url: https://codereview.chromium.org/2685523002
Cr-Commit-Position: refs/heads/master@{#449563}
Committed:
https://chromium.googlesource.com/chromium/src/+/cd18da6b82c146850fc2a10700ae...
==========

[commit-bot: I haz the power, 2017-02-10 07:54:33]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/cd18da6b82c146850fc2a10700ae...