Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10832323:
Adds a UMA histogram to monitor omnibox suggest requests. (Closed)
|
Created:
8 years, 4 months ago by Mark P Modified:
8 years, 4 months ago CC:
chromium-reviews, James Su, msw, hfung Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds a UMA histogram to monitor omnibox suggest requests.
In particular it counts three things.
bucket 1: suggest requests sent
bucket 2: suggest requests invalidated (e.g., due to user typing another character)
bucket 3: suggest responses received
BUG=
TEST=using about:histograms, by hand with various typing speeds
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151913
Patch Set 1 #
Total comments: 2
Patch Set 2 : Reactored according to Ilya's suggestions. #
Total comments: 10
Patch Set 3 : More rearranging. #
Total comments: 1
Patch Set 4 : Altered enum definition. #Messages
Total messages: 14 (0 generated)
Peter, can you review this for correctness? I think I put the code in the right place so it'll capture every event of those three types but I'm not sure. Ilya, can you review this from a metrics perspective? Mike and Harry, consider this an FYI. thanks, mark
https://chromiumcodereview.appspot.com/10832323/diff/1/chrome/browser/autocom... File chrome/browser/autocomplete/search_provider.cc (right): https://chromiumcodereview.appspot.com/10832323/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/search_provider.cc:305: UMA_HISTOGRAM_ENUMERATION("Omnibox.SuggestRequests", 1, 4); To avoid problems from typos and such, there are two best practices for enumerated histograms: (1) Only declare the UMA_HISTOGRAM_ENUMERATION macro in a single location for each distinct histogram. This usually involves defining a helper function like LogOmniboxSuggestRequest(). This makes it much harder to name the histogram differently in different places due to a typo. (This applies to all histograms, not just enumerated ones; but many of the other histograms more naturally appear in just one place in the code.) (2) Use an actual C++ enumeration to name the constants corresponding to "1" and "4". This both helps with readability, makes typos and copy/paste errors more obvious, and makes it easier to extend the list of enumerated values later (since you don't have to remember to change the "4" in the macro). Also, is there any particular reason why you're avoiding use of bucket 0?
ptal https://chromiumcodereview.appspot.com/10832323/diff/1/chrome/browser/autocom... File chrome/browser/autocomplete/search_provider.cc (right): https://chromiumcodereview.appspot.com/10832323/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/search_provider.cc:305: UMA_HISTOGRAM_ENUMERATION("Omnibox.SuggestRequests", 1, 4); On 2012/08/15 22:42:06, Ilya Sherman wrote: > To avoid problems from typos and such, there are two best practices for > enumerated histograms: > > (1) Only declare the UMA_HISTOGRAM_ENUMERATION macro in a single location for > each distinct histogram. This usually involves defining a helper function like > LogOmniboxSuggestRequest(). This makes it much harder to name the histogram > differently in different places due to a typo. (This applies to all histograms, > not just enumerated ones; but many of the other histograms more naturally appear > in just one place in the code.) Good idea. Done. > (2) Use an actual C++ enumeration to name the constants corresponding to "1" and > "4". This both helps with readability, makes typos and copy/paste errors more > obvious, and makes it easier to extend the list of enumerated values later > (since you don't have to remember to change the "4" in the macro). Another good idea. Done. > Also, is there any particular reason why you're avoiding use of bucket 0? I don't like the idea of exploiting the underflow bucket to count things.
UMA usage LGTM. I didn't check the semantics of the logging. > > Also, is there any particular reason why you're avoiding use of bucket 0? > > I don't like the idea of exploiting the underflow bucket to count things. Fair enough. https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... File chrome/browser/autocomplete/search_provider.cc (right): https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... chrome/browser/autocomplete/search_provider.cc:1234: MAX_SUGGEST_REQUEST_HISTOGRAM_VALUE); Optional nit: I think it would be slightly cleaner to indent to the parenthesis. https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... File chrome/browser/autocomplete/search_provider.h (right): https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... chrome/browser/autocomplete/search_provider.h:328: MAX_SUGGEST_REQUEST_HISTOGRAM_VALUE = 4 Optional nit: Explicitly specifying "4" here means you'll have to remember to update it if you ever add more enumerated values. It's up to you, but I'd leave it implicit, so that it's harder for the code to get out of sync in the future. https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... chrome/browser/autocomplete/search_provider.h:332: const SuggestRequestsHistogramValue request_value); nit: No need to declare the parameter as const, since it's a primitive type. https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... chrome/browser/autocomplete/search_provider.h:332: const SuggestRequestsHistogramValue request_value); nit: I would move both of these definitions into an anonymous namespace in the implementation file, unless you need them to be exported to other files (tests?).
https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... File chrome/browser/autocomplete/search_provider.h (right): https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... chrome/browser/autocomplete/search_provider.h:330: // Increments the appropriate value in the histogram by one. nit: blank line to separate enum/function decls.
ptal https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... File chrome/browser/autocomplete/search_provider.cc (right): https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... chrome/browser/autocomplete/search_provider.cc:1234: MAX_SUGGEST_REQUEST_HISTOGRAM_VALUE); On 2012/08/15 23:17:37, Ilya Sherman wrote: > Optional nit: I think it would be slightly cleaner to indent to the parenthesis. Done. https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... File chrome/browser/autocomplete/search_provider.h (right): https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... chrome/browser/autocomplete/search_provider.h:328: MAX_SUGGEST_REQUEST_HISTOGRAM_VALUE = 4 On 2012/08/15 23:17:37, Ilya Sherman wrote: > Optional nit: Explicitly specifying "4" here means you'll have to remember to > update it if you ever add more enumerated values. It's up to you, but I'd leave > it implicit, so that it's harder for the code to get out of sync in the future. Done. I thought of this at the time and then forgot to do it. I think my head isn't screwed on correctly today. https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... chrome/browser/autocomplete/search_provider.h:330: // Increments the appropriate value in the histogram by one. On 2012/08/15 23:21:51, msw wrote: > nit: blank line to separate enum/function decls. Done. https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... chrome/browser/autocomplete/search_provider.h:332: const SuggestRequestsHistogramValue request_value); On 2012/08/15 23:17:37, Ilya Sherman wrote: > nit: No need to declare the parameter as const, since it's a primitive type. Fixed in process of move to .cc file. https://chromiumcodereview.appspot.com/10832323/diff/5001/chrome/browser/auto... chrome/browser/autocomplete/search_provider.h:332: const SuggestRequestsHistogramValue request_value); On 2012/08/15 23:17:37, Ilya Sherman wrote: > nit: I would move both of these definitions into an anonymous namespace in the > implementation file, unless you need them to be exported to other files > (tests?). Moved. (I don't need this function & type exported.)
https://chromiumcodereview.appspot.com/10832323/diff/9001/chrome/browser/auto... File chrome/browser/autocomplete/search_provider.cc (right): https://chromiumcodereview.appspot.com/10832323/diff/9001/chrome/browser/auto... chrome/browser/autocomplete/search_provider.cc:62: REPLY_RECEIVED = 3, Optional nit: Any particular reason to leave the = 2, = 3, etc. explicit? I'm a little worried that it might encourage a pattern like enum SuggestRequestsHistogramValue { REQUEST_SENT = 1, REQUEST_INVALIDATED = 2, REQUEST_INVALIDATED_HARDER = 4, REPLY_RECEIVED = 3, MAX_SUGGEST_REQUEST_HISTOGRAM_VALUE }; which will cause MAX_SUGGEST_REQUEST_HISTOGRAM_VALUE to end up with the wrong value.
On 2012/08/15 23:36:02, Ilya Sherman wrote: > https://chromiumcodereview.appspot.com/10832323/diff/9001/chrome/browser/auto... > File chrome/browser/autocomplete/search_provider.cc (right): > > https://chromiumcodereview.appspot.com/10832323/diff/9001/chrome/browser/auto... > chrome/browser/autocomplete/search_provider.cc:62: REPLY_RECEIVED = 3, > Optional nit: Any particular reason to leave the = 2, = 3, etc. explicit? Good question. The value of these enums had better not change or else our statistics will be all screwed up. That's a good argument for leaving the =2, =3, and so on explicit. Perhaps maybe we should put the =4 back with this argument? > I'm a > little worried that it might encourage a pattern like > > enum SuggestRequestsHistogramValue { > REQUEST_SENT = 1, > REQUEST_INVALIDATED = 2, > REQUEST_INVALIDATED_HARDER = 4, > REPLY_RECEIVED = 3, > MAX_SUGGEST_REQUEST_HISTOGRAM_VALUE > }; > > which will cause MAX_SUGGEST_REQUEST_HISTOGRAM_VALUE to end up with the wrong > value.
On 2012/08/15 23:41:37, Mark P wrote: > On 2012/08/15 23:36:02, Ilya Sherman wrote: > > > https://chromiumcodereview.appspot.com/10832323/diff/9001/chrome/browser/auto... > > File chrome/browser/autocomplete/search_provider.cc (right): > > > > > https://chromiumcodereview.appspot.com/10832323/diff/9001/chrome/browser/auto... > > chrome/browser/autocomplete/search_provider.cc:62: REPLY_RECEIVED = 3, > > Optional nit: Any particular reason to leave the = 2, = 3, etc. explicit? > > Good question. The value of these enums had better not change or else our > statistics will be all screwed up. That's a good argument for leaving the =2, > =3, and so on explicit. Well, it's up to you. I think it's easiest to leave all but the first value implicit, and add a comment (enforced through code review) that additional values should be added to the end of the histogram. If you think explicit values are less likely to run into issues, that's ok with me too; it's just not the pattern I've seen elsewhere, and not the one that I would choose.
On 2012/08/15 23:51:01, Ilya Sherman wrote: > On 2012/08/15 23:41:37, Mark P wrote: > > On 2012/08/15 23:36:02, Ilya Sherman wrote: > > > > > > https://chromiumcodereview.appspot.com/10832323/diff/9001/chrome/browser/auto... > > > File chrome/browser/autocomplete/search_provider.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10832323/diff/9001/chrome/browser/auto... > > > chrome/browser/autocomplete/search_provider.cc:62: REPLY_RECEIVED = 3, > > > Optional nit: Any particular reason to leave the = 2, = 3, etc. explicit? > > > > Good question. The value of these enums had better not change or else our > > statistics will be all screwed up. That's a good argument for leaving the =2, > > =3, and so on explicit. > > Well, it's up to you. I think it's easiest to leave all but the first value > implicit, and add a comment (enforced through code review) that additional > values should be added to the end of the histogram. If you think explicit > values are less likely to run into issues, that's ok with me too; it's just not > the pattern I've seen elsewhere, and not the one that I would choose. New version. Tell me what you think.
LGTM
LGTM, thanks :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/10832323/6005
Change committed as 151913 |
