Add a histogram for omnibox answer suggestion types.

Add a histogram for omnibox answer suggestion types.

BUG=591803
Review-Url: https://codereview.chromium.org/2728663005
Cr-Commit-Position: refs/heads/master@{#454423}
Committed: https://chromium.googlesource.com/chromium/src/+/204c508db463f78401249576c2b5001079da9650

[Justin Donnelly, 2017-03-02 18:59:39]

jdonnelly@chromium.org changed reviewers:
+ mpearson@chromium.org

[Justin Donnelly, 2017-03-02 19:00:13]

https://codereview.chromium.org/2728663005/diff/1/components/omnibox/browser/...
File components/omnibox/browser/search_suggestion_parser.cc (right):

https://codereview.chromium.org/2728663005/diff/1/components/omnibox/browser/...
components/omnibox/browser/search_suggestion_parser.cc:527:
UMA_HISTOGRAM_SPARSE_SLOWLY("Omnibox.AnswerType", answer_type);
Is this the right way to log this? These type IDs used to be sparse values like
2883 and 1372 so this is how I did it in the old Android-only histogram. Now
they're in a new, tight range (currently 1-10). But UMA_HISTOGRAM_ENUMERATION
doesn't seem right because the ID comes from the server so I can't guarantee
it's less than some man enum value. I guess I could define an enum in this file
and if the ID is greater than its max, log an error/0 case? Or is there some
other macro designed for this circumstance?

[Justin Donnelly, 2017-03-02 20:50:38]

jdonnelly@chromium.org changed reviewers:
+ isherman@chromium.org

[Justin Donnelly, 2017-03-02 20:50:39]

+isherman for histograms.xml review

[Mark P, 2017-03-02 20:59:35]

lgtm modulo comments below
(in case you want to submit without another round-trip to me)

Note that I am a metrics/ OWNER so you don't need another review.  That said,
Ilya might have some useful comments on sparse histograms, so I suggest you
leave him on this changelist.

--mark

https://codereview.chromium.org/2728663005/diff/1/components/omnibox/browser/...
File components/omnibox/browser/search_suggestion_parser.cc (right):

https://codereview.chromium.org/2728663005/diff/1/components/omnibox/browser/...
components/omnibox/browser/search_suggestion_parser.cc:527:
UMA_HISTOGRAM_SPARSE_SLOWLY("Omnibox.AnswerType", answer_type);
On 2017/03/02 19:00:13, Justin Donnelly wrote:
> Is this the right way to log this? These type IDs used to be sparse values
like
> 2883 and 1372 so this is how I did it in the old Android-only histogram. Now
> they're in a new, tight range (currently 1-10).

Aren't there almost always zero or only one answer?

> But UMA_HISTOGRAM_ENUMERATION
> doesn't seem right because the ID comes from the server so I can't guarantee
> it's less than some man enum value. I guess I could define an enum in this
file
> and if the ID is greater than its max, log an error/0 case? Or is there some
> other macro designed for this circumstance?

Hopefully sparse histograms are still the right solution.

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

https://codereview.chromium.org/2728663005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:43192: +<histogram
name="Omnibox.AnswerType" enum="SuggestionAnswerType">
Please consider a new name, perhaps one with "Parse" in the name.  (Better
aligns with Omnibox.AnswerParseSuccess histogram, and also doesn't mislead. 
From the name, I would guess this has to do with answers that are displayed.)

https://codereview.chromium.org/2728663005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:43195: +    The number of times each
answer type (e.g. weather, sports score) was
nit: comma after "e.g."

https://codereview.chromium.org/2728663005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:43196: +    received and parsed
successfully.
The way this histogram reads now, it's only useful to compare counts against
each other.  Is there any other way you'd like to compare them, e.g., against
the total number of responses received and parsed successfully?  That is,
normalized.  If not, then nothing to do here.  If so, then either
(a) if there is a histogram that already records this information (I don't think
so, but I'm not sure), add to this histogram description a reference to the
normalizing histogram
or
(b) consider adding the histogram for normalization

[Justin Donnelly, 2017-03-02 21:16:37]

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

[commit-bot: I haz the power, 2017-03-02 21:17:16]

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

[Ilya Sherman, 2017-03-02 21:17:32]

https://codereview.chromium.org/2728663005/diff/1/components/omnibox/browser/...
File components/omnibox/browser/search_suggestion_parser.cc (right):

https://codereview.chromium.org/2728663005/diff/1/components/omnibox/browser/...
components/omnibox/browser/search_suggestion_parser.cc:527:
UMA_HISTOGRAM_SPARSE_SLOWLY("Omnibox.AnswerType", answer_type);
On 2017/03/02 20:59:35, Mark P wrote:
> On 2017/03/02 19:00:13, Justin Donnelly wrote:
> > Is this the right way to log this? These type IDs used to be sparse values
> like
> > 2883 and 1372 so this is how I did it in the old Android-only histogram. Now
> > they're in a new, tight range (currently 1-10).
> 
> Aren't there almost always zero or only one answer?
> 
> > But UMA_HISTOGRAM_ENUMERATION
> > doesn't seem right because the ID comes from the server so I can't guarantee
> > it's less than some man enum value. I guess I could define an enum in this
> file
> > and if the ID is greater than its max, log an error/0 case? Or is there some
> > other macro designed for this circumstance?
> 
> Hopefully sparse histograms are still the right solution.

I recommend sparse histograms over enumerated histograms in most cases where the
histogram isn't being recorded in a tight loop.  So, yeah, I'd imagine that it's
a good choice here.

[Justin Donnelly, 2017-03-02 21:19:49]

On 2017/03/02 21:17:32, Ilya Sherman wrote:
> I recommend sparse histograms over enumerated histograms in most cases where
the
> histogram isn't being recorded in a tight loop.  So, yeah, I'd imagine that
it's
> a good choice here.

Cool, thanks.

[Justin Donnelly, 2017-03-02 21:20:03]

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

[Justin Donnelly, 2017-03-02 21:20:12]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-02 21:20:43]

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

[Justin Donnelly, 2017-03-02 21:21:25]

https://codereview.chromium.org/2728663005/diff/1/components/omnibox/browser/...
File components/omnibox/browser/search_suggestion_parser.cc (right):

https://codereview.chromium.org/2728663005/diff/1/components/omnibox/browser/...
components/omnibox/browser/search_suggestion_parser.cc:527:
UMA_HISTOGRAM_SPARSE_SLOWLY("Omnibox.AnswerType", answer_type);
On 2017/03/02 20:59:35, Mark P wrote:
> Aren't there almost always zero or only one answer?

Yes, there's always zero or one. When there's an answer, it includes an int type
and that type is in the range of 1-10 (defined as AnswerTriggererKey in
http://google3/logs/proto/suggest/suggest_extensions.proto).

Is there an implication of fact that there's only 0 or 1 answers I'm not
accounting for here?

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

https://codereview.chromium.org/2728663005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:43192: +<histogram
name="Omnibox.AnswerType" enum="SuggestionAnswerType">
On 2017/03/02 20:59:35, Mark P wrote:
> Please consider a new name, perhaps one with "Parse" in the name.  (Better
> aligns with Omnibox.AnswerParseSuccess histogram, and also doesn't mislead. 
> From the name, I would guess this has to do with answers that are displayed.)

Done.

https://codereview.chromium.org/2728663005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:43195: +    The number of times each
answer type (e.g. weather, sports score) was
On 2017/03/02 20:59:35, Mark P wrote:
> nit: comma after "e.g."

Done.

https://codereview.chromium.org/2728663005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:43196: +    received and parsed
successfully.
On 2017/03/02 20:59:35, Mark P wrote:
> The way this histogram reads now, it's only useful to compare counts against
> each other.  Is there any other way you'd like to compare them, e.g., against
> the total number of responses received and parsed successfully?  That is,
> normalized.  If not, then nothing to do here.  If so, then either
> (a) if there is a histogram that already records this information (I don't
think
> so, but I'm not sure), add to this histogram description a reference to the
> normalizing histogram
> or
> (b) consider adding the histogram for normalization

Done (a). Can you double-check that what I wrote makes sense and addresses your
comment? I'm a little confused because you mentioned AnswerParseSuccess in an
earlier comment but then stated here that you don't think a histogram that
records the number of responses parsed successfully. I'm worried I'm missing
something. :)

[commit-bot: I haz the power, 2017-03-02 22:21:07]

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

[commit-bot: I haz the power, 2017-03-02 22:21:08]

Dry run: This issue passed the CQ dry run.

[Justin Donnelly, 2017-03-02 23:08:56]

Submitting now. If you have any further thoughts on my questions above, I'd be
happy to follow up in another CL.

[Justin Donnelly, 2017-03-02 23:09:02]

The CQ bit was checked by jdonnelly@chromium.org

[Justin Donnelly, 2017-03-02 23:09:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2728663005/#ps40001
(title: "Respond to comments.")

[commit-bot: I haz the power, 2017-03-02 23:09:24]

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-03-02 23:14:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488496142450990,
"parent_rev": "edbdb355635430d400178d6d62d2880ab8f0af68", "commit_rev":
"204c508db463f78401249576c2b5001079da9650"}

[commit-bot: I haz the power, 2017-03-02 23:15:22]

Description was changed from

==========
Add a histogram for omnibox answer suggestion types.

BUG=591803
==========

to

==========
Add a histogram for omnibox answer suggestion types.

BUG=591803

Review-Url: https://codereview.chromium.org/2728663005
Cr-Commit-Position: refs/heads/master@{#454423}
Committed:
https://chromium.googlesource.com/chromium/src/+/204c508db463f78401249576c2b5...
==========

[commit-bot: I haz the power, 2017-03-02 23:15:23]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/204c508db463f78401249576c2b5...

[Mark P, 2017-03-02 23:28:48]

https://codereview.chromium.org/2728663005/diff/1/components/omnibox/browser/...
File components/omnibox/browser/search_suggestion_parser.cc (right):

https://codereview.chromium.org/2728663005/diff/1/components/omnibox/browser/...
components/omnibox/browser/search_suggestion_parser.cc:527:
UMA_HISTOGRAM_SPARSE_SLOWLY("Omnibox.AnswerType", answer_type);
On 2017/03/02 21:21:25, Justin Donnelly wrote:
> On 2017/03/02 20:59:35, Mark P wrote:
> > Aren't there almost always zero or only one answer?
> 
> Yes, there's always zero or one. When there's an answer, it includes an int
type
> and that type is in the range of 1-10 (defined as AnswerTriggererKey in
> http://google3/logs/proto/suggest/suggest_extensions.proto).
> 
> Is there an implication of fact that there's only 0 or 1 answers I'm not
> accounting for here?

No "implication."  It just means that you're not calling
UMA_HISTOGRAM_SPARSE_SLOWLY() repeatedly in a tight loop.  You're calling it at
most once, so its possible slowness doesn't matter.

https://codereview.chromium.org/2728663005/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2728663005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:43197: +    Omnibox.AnswerParseSuccess.
For the follow-up changelist:
Normalizing by Omnibox.AnswerParseSuccess is unnecessary.  You can normalize by
the total number of items emitted to this histogram.

What I want to know was whether you were curious about a different normalization
factor ("e.g., against the total number of responses received and parsed
successfully"); see my original comment.
On 2017/03/02 21:21:25, Justin Donnelly wrote:
> On 2017/03/02 20:59:35, Mark P wrote:
> > The way this histogram reads now, it's only useful to compare counts against
> > each other.  Is there any other way you'd like to compare them, e.g.,
against
> > the total number of responses received and parsed successfully?  That is,
> > normalized.  If not, then nothing to do here.  If so, then either
> > (a) if there is a histogram that already records this information (I don't
> think
> > so, but I'm not sure), add to this histogram description a reference to the
> > normalizing histogram
> > or
> > (b) consider adding the histogram for normalization
> 
> Done (a). Can you double-check that what I wrote makes sense and addresses
your
> comment? I'm a little confused because you mentioned AnswerParseSuccess in an
> earlier comment but then stated here that you don't think a histogram that
> records the number of responses parsed successfully. I'm worried I'm missing
> something. :)

You're right; you were missing something. :-P

[Mark P, 2017-03-02 23:41:04]

One more thought -

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

https://codereview.chromium.org/2728663005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:109792: +  <summary>Type of Answer shown
in omnibox suggestion list.</summary>
Where is the suggest server code for this?  Can you make sure it has the
standard warning label about not being able to change the values?
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...

[Justin Donnelly, 2017-03-03 00:39:32]

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

https://codereview.chromium.org/2728663005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:109792: +  <summary>Type of Answer shown
in omnibox suggestion list.</summary>
On 2017/03/02 23:41:04, Mark P wrote:
> Where is the suggest server code for this?  Can you make sure it has the
> standard warning label about not being able to change the values?
>
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...

This is AnswerTriggererKey in
http://google3/logs/proto/suggest/suggest_extensions.proto. I'll add the
warning.

https://codereview.chromium.org/2728663005/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2728663005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:43197: +    Omnibox.AnswerParseSuccess.
On 2017/03/02 23:28:48, Mark P wrote:
> For the follow-up changelist:
> Normalizing by Omnibox.AnswerParseSuccess is unnecessary.  You can normalize
by
> the total number of items emitted to this histogram.
> 
> What I want to know was whether you were curious about a different
normalization
> factor ("e.g., against the total number of responses received and parsed
> successfully"); see my original comment.
> On 2017/03/02 21:21:25, Justin Donnelly wrote:
> > On 2017/03/02 20:59:35, Mark P wrote:
> > > The way this histogram reads now, it's only useful to compare counts
against
> > > each other.  Is there any other way you'd like to compare them, e.g.,
> against
> > > the total number of responses received and parsed successfully?  That is,
> > > normalized.  If not, then nothing to do here.  If so, then either
> > > (a) if there is a histogram that already records this information (I don't
> > think
> > > so, but I'm not sure), add to this histogram description a reference to
the
> > > normalizing histogram
> > > or
> > > (b) consider adding the histogram for normalization
> > 
> > Done (a). Can you double-check that what I wrote makes sense and addresses
> your
> > comment? I'm a little confused because you mentioned AnswerParseSuccess in
an
> > earlier comment but then stated here that you don't think a histogram that
> > records the number of responses parsed successfully. I'm worried I'm missing
> > something. :)
> 
> You're right; you were missing something. :-P

Hmm, I think I'm still missing it. Based on the logging code in
search_suggestion_parser.cc the total number of items emitted to this histogram
should be the same thing as the number of responses parsed successfully. But if
it's not, why isn't Omnibox.AnswerParseSuccess that value? Is it because it's a
binary histogram (success/failure)? Like it would need to be a separate
non-histogram metric?

[Mark P, 2017-03-03 04:53:20]

https://codereview.chromium.org/2728663005/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2728663005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:43197: +    Omnibox.AnswerParseSuccess.
On 2017/03/03 00:39:32, Justin Donnelly wrote:
> On 2017/03/02 23:28:48, Mark P wrote:
> > For the follow-up changelist:
> > Normalizing by Omnibox.AnswerParseSuccess is unnecessary.  You can normalize
> by
> > the total number of items emitted to this histogram.
> > 
> > What I want to know was whether you were curious about a different
> normalization
> > factor ("e.g., against the total number of responses received and parsed
> > successfully"); see my original comment.
> > On 2017/03/02 21:21:25, Justin Donnelly wrote:
> > > On 2017/03/02 20:59:35, Mark P wrote:
> > > > The way this histogram reads now, it's only useful to compare counts
> against
> > > > each other.  Is there any other way you'd like to compare them, e.g.,
> > against
> > > > the total number of responses received and parsed successfully?  That
is,
> > > > normalized.  If not, then nothing to do here.  If so, then either
> > > > (a) if there is a histogram that already records this information (I
don't
> > > think
> > > > so, but I'm not sure), add to this histogram description a reference to
> the
> > > > normalizing histogram
> > > > or
> > > > (b) consider adding the histogram for normalization
> > > 
> > > Done (a). Can you double-check that what I wrote makes sense and addresses
> > your
> > > comment? I'm a little confused because you mentioned AnswerParseSuccess in
> an
> > > earlier comment but then stated here that you don't think a histogram that
> > > records the number of responses parsed successfully. I'm worried I'm
missing
> > > something. :)
> > 
> > You're right; you were missing something. :-P
> 
> Hmm, I think I'm still missing it. Based on the logging code in
> search_suggestion_parser.cc the total number of items emitted to this
histogram
> should be the same thing as the number of responses parsed successfully.

Do we have a histogram for total number of responses parsed successfully?  As
far as I see, we only have a histogram for total number of answers suggestions
parsed successfully.

> it's not, why isn't Omnibox.AnswerParseSuccess that value? Is it because it's
a
> binary histogram (success/failure)? Like it would need to be a separate
> non-histogram metric?

[Justin Donnelly, 2017-03-03 14:47:52]

On 2017/03/03 04:53:20, Mark P wrote:
> Do we have a histogram for total number of responses parsed successfully?  As
> far as I see, we only have a histogram for total number of answers suggestions
> parsed successfully.

Ah, ok. I kept hearing you say "total responses" and thinking you were referring
to total answer responses. How about Omnibox.SuggestRequests' REPLY_RECEIVED?

[Mark P, 2017-03-03 18:40:44]

On Fri, Mar 3, 2017 at 6:47 AM, <jdonnelly@chromium.org> wrote:

> On 2017/03/03 04:53:20, Mark P wrote:
> > Do we have a histogram for total number of responses parsed
> successfully? As
> > far as I see, we only have a histogram for total number of answers
> suggestions
> > parsed successfully.
>
> Ah, ok. I kept hearing you say "total responses" and thinking you were
> referring
> to total answer responses. How about Omnibox.SuggestRequests'
> REPLY_RECEIVED?
>

That's close, yet not quite right.  It doesn't say whether the request was
successful nor whether the results were parsed correctly.

Closer still is the count of emits to
Omnibox.SuggestRequest.Success.GoogleResponseTime, which is Google-only
successful requests.

It also doesn't include anything about whether we successfully extract,
deserialize, and parse the JSON, but I think that's the best we have.

Can you mention using the count of
Omnibox.SuggestRequest.Success.GoogleResponseTime as a normalization for
all requests that might be able to have answers in the histogram
description?  And point out that we're off under those conditions (extract,
deserialize, and parse), but that's normally not a problem because the
error is probably pretty small (or non-existent)--we trust our servers.

--mark


> https://codereview.chromium.org/2728663005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Justin Donnelly, 2017-03-03 18:58:59]

On Fri, Mar 3, 2017 at 10:40 AM, Mark Pearson <mpearson@chromium.org> wrote:

> Can you mention using the count of
Omnibox.SuggestRequest.Success.GoogleResponseTime
> as a normalization for all requests that might be able to have answers in
> the histogram description?  And point out that we're off under those
> conditions (extract, deserialize, and parse), but that's normally not a
> problem because the error is probably pretty small (or non-existent)--we
> trust our servers.
>

Makes sense, will do.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Justin Donnelly, 2017-03-03 22:03:33]

Patchset #3 (id:60001) has been deleted