Contextual search - clean up histogram.xml metric definitions.

Clean up histogram.xml metric definitions.

These metrics were never logged.

BUG=341765
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290958

[manzagop (departed), 2014-08-19 16:38:05]

Hi guys!
This cl introduces the xml changes for the contextual search histograms. Please
have a look.
Thanks,
Pierre

[donnd, 2014-08-19 16:55:10]

Looking quite good!  

Just a few questions, and I'd like other reviewers to look too.

https://codereview.chromium.org/486303004/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/486303004/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:28273: +    The details (origin state
and reason) of the first entry into the closed
Nit: "origin state" seems awkward, maybe just say "previous state" instead?

https://codereview.chromium.org/486303004/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:28354: +    search that started with the
first run flow.
This is confusing to me because it sounds like it only logs the exit when the
user says yes, otherwise there wouldn't be a search.  But I would think we'd
want to log the state of the preference after the user made any decision to
opt-in or opt-out.

https://codereview.chromium.org/486303004/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:28363: +    The state of the Contextual
Search Preference. Can be logged multiple times.
It's great that this can be logged multiple times!  I presume there's some
de-dupe logic?  I know it's not from this CL but can you explain how that works?

https://codereview.chromium.org/486303004/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:28373: +    The Contextual Search
preference state after a modification from the the
Typo "the the"

[manzagop (departed), 2014-08-19 17:45:58]

Addressed comments. PTAL.

https://codereview.chromium.org/486303004/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/486303004/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:28273: +    The details (origin state
and reason) of the first entry into the closed
On 2014/08/19 16:55:10, Donn wrote:
> Nit: "origin state" seems awkward, maybe just say "previous state" instead?

Done.

https://codereview.chromium.org/486303004/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:28354: +    search that started with the
first run flow.
On 2014/08/19 16:55:10, Donn wrote:
> This is confusing to me because it sounds like it only logs the exit when the
> user says yes, otherwise there wouldn't be a search.  But I would think we'd
> want to log the state of the preference after the user made any decision to
> opt-in or opt-out.

Done.

https://codereview.chromium.org/486303004/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:28363: +    The state of the Contextual
Search Preference. Can be logged multiple times.
On 2014/08/19 16:55:10, Donn wrote:
> It's great that this can be logged multiple times!  I presume there's some
> de-dupe logic?  I know it's not from this CL but can you explain how that
works?

This is something that UMA provides (the number of users that appear in a given
histogram bin). I mention it here to ensure people looking at this know they
should use the "user" view, not the default raw event count view.

https://codereview.chromium.org/486303004/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:28373: +    The Contextual Search
preference state after a modification from the the
On 2014/08/19 16:55:10, Donn wrote:
> Typo "the the"

Done.

[donnd, 2014-08-19 17:51:25]

Thanks for explaining the user-view!

LGTM, but I'd like Mathieu or Pedro to review too.

[Mathieu, 2014-08-19 17:53:46]

+asvitkine

Alexei's probably going to want to see your client code exercising this, so
perhaps include change numbers in the description.

[manzagop (departed), 2014-08-19 18:11:15]

On 2014/08/19 17:53:46, Mathieu Perreault wrote:
> +asvitkine
> 
> Alexei's probably going to want to see your client code exercising this, so
> perhaps include change numbers in the description.

Done.

[Alexei Svitkine (slow), 2014-08-19 18:14:15]

If these histograms are implemented in the internal repo, would it make more
sense to add them to the google3 version of histograms.xml?

[pedro (no code reviews), 2014-08-19 18:33:29]

lgtm

[manzagop (departed), 2014-08-19 18:58:18]

Ok, will relocate them.


On Tue, Aug 19, 2014 at 2:33 PM, <pedrosimonetti@chromium.org> wrote:

> lgtm
>
>
>
> https://codereview.chromium.org/486303004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[manzagop (departed), 2014-08-19 19:44:01]

On 2014/08/19 18:58:18, manzagop wrote:
> Ok, will relocate them.
> 
> 
> On Tue, Aug 19, 2014 at 2:33 PM, <mailto:pedrosimonetti@chromium.org> wrote:
> 
> > lgtm
> >
> >
> >
> > https://codereview.chromium.org/486303004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Changed this CL to a cleanup one. Please have another look!

[Alexei Svitkine (slow), 2014-08-20 14:42:43]

https://codereview.chromium.org/486303004/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/486303004/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:28244: -<histogram
name="Search.ContextualSearchOptPeekCard"
I don't see these being added in another CL to the internal file.

Please don't delete existing histograms definitions (unless we never ever saw
any data for them - e.g. the corresponding code didn't exist or wasn't hit).

Instead, the best practice is to mark them as <obsolete> in the file.

In this case, since they're moving to the internal file, you can first move them
offer (i.e. with a corresponding google3 CL that re-adds these) and then in a
follow-up CL, annotate the ones that are obsolete as such.

[manzagop (departed), 2014-08-20 14:51:44]

On 2014/08/20 14:42:43, Alexei Svitkine wrote:
>
https://codereview.chromium.org/486303004/diff/40001/tools/metrics/histograms...
> File tools/metrics/histograms/histograms.xml (left):
> 
>
https://codereview.chromium.org/486303004/diff/40001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:28244: -<histogram
> name="Search.ContextualSearchOptPeekCard"
> I don't see these being added in another CL to the internal file.
> 
> Please don't delete existing histograms definitions (unless we never ever saw
> any data for them - e.g. the corresponding code didn't exist or wasn't hit).
> 
> Instead, the best practice is to mark them as <obsolete> in the file.
> 
> In this case, since they're moving to the internal file, you can first move
them
> offer (i.e. with a corresponding google3 CL that re-adds these) and then in a
> follow-up CL, annotate the ones that are obsolete as such.

Pedro: was data ever logged for these histograms?

[pedro (no code reviews), 2014-08-20 17:32:25]

On 2014/08/20 14:51:44, manzagop wrote:
> On 2014/08/20 14:42:43, Alexei Svitkine wrote:
> >
>
https://codereview.chromium.org/486303004/diff/40001/tools/metrics/histograms...
> > File tools/metrics/histograms/histograms.xml (left):
> > 
> >
>
https://codereview.chromium.org/486303004/diff/40001/tools/metrics/histograms...
> > tools/metrics/histograms/histograms.xml:28244: -<histogram
> > name="Search.ContextualSearchOptPeekCard"
> > I don't see these being added in another CL to the internal file.
> > 
> > Please don't delete existing histograms definitions (unless we never ever
saw
> > any data for them - e.g. the corresponding code didn't exist or wasn't hit).
> > 
> > Instead, the best practice is to mark them as <obsolete> in the file.
> > 
> > In this case, since they're moving to the internal file, you can first move
> them
> > offer (i.e. with a corresponding google3 CL that re-adds these) and then in
a
> > follow-up CL, annotate the ones that are obsolete as such.
> 
> Pedro: was data ever logged for these histograms?

I don't think so. We've created them for a simplified JavaScript prototype, but
soon after that we changed our plans to implement the whole feature natively.

[Alexei Svitkine (slow), 2014-08-20 18:57:17]

Thanks, I checked on the dashboard and indeed see no data for them. LGTM!

[Alexei Svitkine (slow), 2014-08-20 18:57:44]

Maybe expand the CL description to mention these were never logged.

[manzagop (departed), 2014-08-20 19:06:37]

On 2014/08/20 18:57:44, Alexei Svitkine wrote:
> Maybe expand the CL description to mention these were never logged.

Thanks for the review!

[manzagop (departed), 2014-08-20 19:06:49]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2014-08-20 19:08:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/manzagop@chromium.org/486303004/40001

[commit-bot: I haz the power, 2014-08-20 20:44:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-20 22:08:39]

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

[commit-bot: I haz the power, 2014-08-20 22:08:40]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mathieu, 2014-08-20 22:11:30]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-08-20 22:13:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/manzagop@chromium.org/486303004/40001

[commit-bot: I haz the power, 2014-08-20 23:21:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-21 00:11:09]

Committed patchset #3 (40001) as 290958