Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2619203007:
Log suggestion scores in 10 discrete buckets. (Closed)
DescriptionLog suggestion scores in 10 discrete buckets.
Quality scores are usually in [0,1]. As UMA histograms don't support floats,
this CL changes the logging to log as integer in [1,10].
Histogram definitions cannot be changed, so this CL deprecates the old Score histograms and introduces new ScoreNormalized histograms.
BUG=672525
Review-Url: https://codereview.chromium.org/2619203007
Cr-Commit-Position: refs/heads/master@{#444321}
Committed: https://chromium.googlesource.com/chromium/src/+/ec4069a22ddb364e0f28adc4f4f5f740a477d3a0
Patch Set 1 #
Total comments: 6
Patch Set 2 : deprecate old score histograms and introduce new, normalized ones #
Total comments: 2
Patch Set 3 : clarified histogram description #Patch Set 4 : fixed float literals #
Total comments: 4
Patch Set 5 : reuse functionality from histogram_functions.h #
Total comments: 4
Patch Set 6 : cleaned up metrics code #
Total comments: 9
Patch Set 7 : more comments #
Total comments: 5
Patch Set 8 : introduce histogram enum and clarified comment. #Patch Set 9 : fixed enum values #
Messages
Total messages: 52 (24 generated)
tschumann@chromium.org changed reviewers: + jkrcal@chromium.org
https://codereview.chromium.org/2619203007/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:173: // floats, we put them on a discrete scale of [1,10]. We keep the extra bucket nit: Could you expand the comment, e.g. "For instance, discrete value 1 represents values (0.0, 0.1] and value 10 represents values (0.9, 1.0]." Could you also put the same explanation in the definition in histograms.xml? https://codereview.chromium.org/2619203007/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:175: base::Histogram::FactoryGet(name, 1, 11, 12, I think you should use LinearHistogram, instead. Histogram uses exponential scale for buckets. This may be the same as the linear one when number of buckets = cardinality of the range but I think using Linear directly is clearer. I got puzzled by having max_value = 11. Is not (name, 1, 10, 11, ..) enough? Does the unit-test fail? Additionally, I do not think you can change type of a histogram without renaming it. Did you try to run Chrome with your patch? https://codereview.chromium.org/2619203007/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_metrics_unittest.cc (right): https://codereview.chromium.org/2619203007/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics_unittest.cc:20: base::HistogramTester histogram_tester; Yay for increasing test coverage! And thanks for learning me something new, I never used HistogramTester :)
https://codereview.chromium.org/2619203007/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:173: // floats, we put them on a discrete scale of [1,10]. We keep the extra bucket On 2017/01/10 19:31:47, jkrcal wrote: > nit: Could you expand the comment, e.g. "For instance, discrete value 1 > represents values (0.0, 0.1] and value 10 represents values (0.9, 1.0]." > > Could you also put the same explanation in the definition in histograms.xml? Done. https://codereview.chromium.org/2619203007/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:175: base::Histogram::FactoryGet(name, 1, 11, 12, On 2017/01/10 19:31:47, jkrcal wrote: > I think you should use LinearHistogram, instead. Histogram uses exponential > scale for buckets. This may be the same as the linear one when > number of buckets = cardinality of the range > but I think using Linear directly is clearer. > > I got puzzled by having max_value = 11. Is not (name, 1, 10, 11, ..) enough? > Does the unit-test fail? > > Additionally, I do not think you can change type of a histogram without renaming > it. Did you try to run Chrome with your patch? Apparently, we need to create a new histogram for this :-/ (https://groups.google.com/a/google.com/forum/#!msg/uma-users/L-aaSsAMMaM/CNC1...) Done and also changed to LinearHistogram. I added an explicit overflow bucket so that we can distinguish a score of 0.91 from something weird like a score of 42.
lgtm, thanks! https://codereview.chromium.org/2619203007/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:175: base::Histogram::FactoryGet(name, 1, 11, 12, On 2017/01/12 12:25:31, tschumann wrote: > On 2017/01/10 19:31:47, jkrcal wrote: > > I think you should use LinearHistogram, instead. Histogram uses exponential > > scale for buckets. This may be the same as the linear one when > > number of buckets = cardinality of the range > > but I think using Linear directly is clearer. > > > > I got puzzled by having max_value = 11. Is not (name, 1, 10, 11, ..) enough? > > Does the unit-test fail? > > > > Additionally, I do not think you can change type of a histogram without > renaming > > it. Did you try to run Chrome with your patch? > > Apparently, we need to create a new histogram for this :-/ > (https://groups.google.com/a/google.com/forum/#!msg/uma-users/L-aaSsAMMaM/CNC1...) > > Done and also changed to LinearHistogram. > > I added an explicit overflow bucket so that we can distinguish a score of 0.91 > from something weird like a score of 42. Acknowledged. https://codereview.chromium.org/2619203007/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2619203007/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:38807: + (0.0, 0.1]. nit: Please explain also the overflows here for all the new histograms. These overflow values can appear in the UMA dashboard so it is important to understand what this bucket means.
Description was changed from ========== Log suggestion scores in 10 discrete buckets. Quality scores are usually in [0,1]. As UMA histograms don't support floats, this CL changes the logging to log as integer in [1,10]. BUG=672525 ========== to ========== Log suggestion scores in 10 discrete buckets. Quality scores are usually in [0,1]. As UMA histograms don't support floats, this CL changes the logging to log as integer in [1,10]. Histogram definitions cannot be changed, so this CL deprecates the old Score histograms and introduces new ScoreNormalized histograms. BUG=672525 ==========
tschumann@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine for histograms.xml https://codereview.chromium.org/2619203007/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2619203007/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:38807: + (0.0, 0.1]. On 2017/01/12 15:13:10, jkrcal wrote: > nit: Please explain also the overflows here for all the new histograms. These > overflow values can appear in the UMA dashboard so it is important to understand > what this bucket means. Done.
The CQ bit was checked by tschumann@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tschumann@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...
https://codereview.chromium.org/2619203007/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:180: ->Add(ceil(value * 10)); Can you use UmaHistogramExactLinear() here? https://codereview.chromium.org/2619203007/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2619203007/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:109721: - <affected-histogram name="NewTabPage.ContentSuggestions.MenuOpenedScore"/> Keep the existing entries here, even if they're deprecated. As this is still needed for UMA infrastructure to know about the previously logged metrics.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
thanks! https://codereview.chromium.org/2619203007/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:180: ->Add(ceil(value * 10)); On 2017/01/12 18:42:04, Alexei Svitkine (slow) wrote: > Can you use UmaHistogramExactLinear() here? Yes. That's actually nice! Turns out UmaHistogramLongTimes() already exists there too (I could use another pair of eyes double-checking they are indeed identical). https://codereview.chromium.org/2619203007/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2619203007/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:109721: - <affected-histogram name="NewTabPage.ContentSuggestions.MenuOpenedScore"/> On 2017/01/12 18:42:05, Alexei Svitkine (slow) wrote: > Keep the existing entries here, even if they're deprecated. As this is still > needed for UMA infrastructure to know about the previously logged metrics. Done.
The CQ bit was checked by tschumann@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...
https://codereview.chromium.org/2619203007/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:143: void UmaHistogramEnumeration(const std::string& name, Can this be removed too? https://codereview.chromium.org/2619203007/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:186: UmaHistogramLongTimes(name, value); Prefix this with base:: ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2619203007/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:143: void UmaHistogramEnumeration(const std::string& name, On 2017/01/13 18:40:00, Alexei Svitkine (slow) wrote: > Can this be removed too? yes -- thanks for calling that out! Turned out we abused them quite a bit. Changed most callers to use the ExactLinear counter parts. https://codereview.chromium.org/2619203007/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:186: UmaHistogramLongTimes(name, value); On 2017/01/13 18:40:00, Alexei Svitkine (slow) wrote: > Prefix this with base:: ? Done. (ADL at it's best ;-)).
+Marc do you remember why we use (WindowOpenDisposition::MAX_VALUE) + 1) as an upper boundary for the histograms?
On 2017/01/16 14:14:19, tschumann wrote: > +Marc do you remember why we use > (WindowOpenDisposition::MAX_VALUE) + 1) as an upper boundary for the histograms? I think that's just what the histogram macros expect - a count, not a max value.
treib@chromium.org changed reviewers: + treib@chromium.org
https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:269: UMA_HISTOGRAM_EXACT_LINEAR( I don't understand this change - what was wrong with UMA_HISTOGRAM_ENUMERATION here?
On 2017/01/16 14:35:00, Marc Treib wrote: > On 2017/01/16 14:14:19, tschumann wrote: > > +Marc do you remember why we use > > (WindowOpenDisposition::MAX_VALUE) + 1) as an upper boundary for the > histograms? > > I think that's just what the histogram macros expect - a count, not a max value. Hehe, actually it seems they expect the max value: https://codesearch.chromium.org/chromium/src/base/metrics/histogram_functions... They do the increment internally. Given that this bucket was never used, do you think we can adjust the boundaries?
https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:269: UMA_HISTOGRAM_EXACT_LINEAR( On 2017/01/16 14:35:12, Marc Treib wrote: > I don't understand this change - what was wrong with UMA_HISTOGRAM_ENUMERATION > here? well, we're not passing in an enum :-) base::UmaHistogramEnumeration (which I actually wanted to use) enforces the types being enums.
Huh. Two options: 1) I got confused. 2) The explicit +1 is to have an overflow bucket, in case enum values are added later etc. https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:269: UMA_HISTOGRAM_EXACT_LINEAR( On 2017/01/16 15:00:52, tschumann wrote: > On 2017/01/16 14:35:12, Marc Treib wrote: > > I don't understand this change - what was wrong with UMA_HISTOGRAM_ENUMERATION > > here? > > well, we're not passing in an enum :-) > base::UmaHistogramEnumeration (which I actually wanted to use) enforces the > types being enums. But we *are* passing in an enum here...? I guess the function doesn't work with enum classes? That seems like something to fix there...
https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:269: UMA_HISTOGRAM_EXACT_LINEAR( On 2017/01/16 15:12:24, Marc Treib wrote: > On 2017/01/16 15:00:52, tschumann wrote: > > On 2017/01/16 14:35:12, Marc Treib wrote: > > > I don't understand this change - what was wrong with > UMA_HISTOGRAM_ENUMERATION > > > here? > > > > well, we're not passing in an enum :-) > > base::UmaHistogramEnumeration (which I actually wanted to use) enforces the > > types being enums. > > But we *are* passing in an enum here...? I guess the function doesn't work with > enum classes? That seems like something to fix there... nope, at least strictly speaking we're passing two integers. The max_value parameter is not within the enum range anymore. This is very theoretical of course, as these macros also turn the values into integers. The macro is not working well with enum classes as they assume it's implicitly cast-able to integers (they simply add 1). Not sure if the function UmaHistogramEnumeration() works with enum classes. I'm fine with having the casts here and using the ENUM version of the logging macros/functions, but the special treatment of max_value made me wonder. If you think it's safe to reduce the upper boundary, then I'm happy to change this to use the enum version (and add a comment in case we need the cast). Adding new enum values should always be ok and needs no special treatment (https://codesearch.chromium.org/chromium/src/base/metrics/histogram_macros.h?...).
On 2017/01/16 15:27:32, tschumann wrote: > https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... > File components/ntp_snippets/content_suggestions_metrics.cc (right): > > https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... > components/ntp_snippets/content_suggestions_metrics.cc:269: > UMA_HISTOGRAM_EXACT_LINEAR( > On 2017/01/16 15:12:24, Marc Treib wrote: > > On 2017/01/16 15:00:52, tschumann wrote: > > > On 2017/01/16 14:35:12, Marc Treib wrote: > > > > I don't understand this change - what was wrong with > > UMA_HISTOGRAM_ENUMERATION > > > > here? > > > > > > well, we're not passing in an enum :-) > > > base::UmaHistogramEnumeration (which I actually wanted to use) enforces the > > > types being enums. > > > > But we *are* passing in an enum here...? I guess the function doesn't work > with > > enum classes? That seems like something to fix there... > > nope, at least strictly speaking we're passing two integers. The max_value > parameter is not within the enum range anymore. > This is very theoretical of course, as these macros also turn the values into > integers. > > The macro is not working well with enum classes as they assume it's implicitly > cast-able to integers (they simply add 1). Not sure if the function > UmaHistogramEnumeration() works with enum classes. > > I'm fine with having the casts here and using the ENUM version of the logging > macros/functions, but the special treatment of max_value made me wonder. If you > think it's safe to reduce the upper boundary, then I'm happy to change this to > use the enum version (and add a comment in case we need the cast). > > Adding new enum values should always be ok and needs no special treatment > (https://codesearch.chromium.org/chromium/src/base/metrics/histogram_macros.h?...). Hah, from that same link: "The value in |sample| must be strictly less than |enum_max|." Maybe that comment is incorrect (asvitkine should know), but I'm guessing this is why the "+1" is there.
On 2017/01/16 15:48:03, Marc Treib wrote: > On 2017/01/16 15:27:32, tschumann wrote: > > > https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... > > File components/ntp_snippets/content_suggestions_metrics.cc (right): > > > > > https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... > > components/ntp_snippets/content_suggestions_metrics.cc:269: > > UMA_HISTOGRAM_EXACT_LINEAR( > > On 2017/01/16 15:12:24, Marc Treib wrote: > > > On 2017/01/16 15:00:52, tschumann wrote: > > > > On 2017/01/16 14:35:12, Marc Treib wrote: > > > > > I don't understand this change - what was wrong with > > > UMA_HISTOGRAM_ENUMERATION > > > > > here? > > > > > > > > well, we're not passing in an enum :-) > > > > base::UmaHistogramEnumeration (which I actually wanted to use) enforces > the > > > > types being enums. > > > > > > But we *are* passing in an enum here...? I guess the function doesn't work > > with > > > enum classes? That seems like something to fix there... > > > > nope, at least strictly speaking we're passing two integers. The max_value > > parameter is not within the enum range anymore. > > This is very theoretical of course, as these macros also turn the values into > > integers. > > > > The macro is not working well with enum classes as they assume it's implicitly > > cast-able to integers (they simply add 1). Not sure if the function > > UmaHistogramEnumeration() works with enum classes. > > > > I'm fine with having the casts here and using the ENUM version of the logging > > macros/functions, but the special treatment of max_value made me wonder. If > you > > think it's safe to reduce the upper boundary, then I'm happy to change this to > > use the enum version (and add a comment in case we need the cast). > > > > Adding new enum values should always be ok and needs no special treatment > > > (https://codesearch.chromium.org/chromium/src/base/metrics/histogram_macros.h?...). > > Hah, from that same link: "The value in |sample| must be strictly less than > |enum_max|." Maybe that comment is incorrect (asvitkine should know), but I'm > guessing this is why the "+1" is there. I guess the underlying issue is that MAX_VALUE or COUNT entries in enums are never supposed to be used for actual values but just as sentinels. So they should never show up in the logs. +Alexei, can you clarify?
That's correct. On Mon, Jan 16, 2017 at 10:50 AM, <tschumann@chromium.org> wrote: > On 2017/01/16 15:48:03, Marc Treib wrote: > > On 2017/01/16 15:27:32, tschumann wrote: > > > > > > https://codereview.chromium.org/2619203007/diff/100001/ > components/ntp_snippets/content_suggestions_metrics.cc > > > File components/ntp_snippets/content_suggestions_metrics.cc (right): > > > > > > > > > https://codereview.chromium.org/2619203007/diff/100001/ > components/ntp_snippets/content_suggestions_metrics.cc#newcode269 > > > components/ntp_snippets/content_suggestions_metrics.cc:269: > > > UMA_HISTOGRAM_EXACT_LINEAR( > > > On 2017/01/16 15:12:24, Marc Treib wrote: > > > > On 2017/01/16 15:00:52, tschumann wrote: > > > > > On 2017/01/16 14:35:12, Marc Treib wrote: > > > > > > I don't understand this change - what was wrong with > > > > UMA_HISTOGRAM_ENUMERATION > > > > > > here? > > > > > > > > > > well, we're not passing in an enum :-) > > > > > base::UmaHistogramEnumeration (which I actually wanted to use) > enforces > > the > > > > > types being enums. > > > > > > > > But we *are* passing in an enum here...? I guess the function > doesn't work > > > with > > > > enum classes? That seems like something to fix there... > > > > > > nope, at least strictly speaking we're passing two integers. The > max_value > > > parameter is not within the enum range anymore. > > > This is very theoretical of course, as these macros also turn the > values > into > > > integers. > > > > > > The macro is not working well with enum classes as they assume it's > implicitly > > > cast-able to integers (they simply add 1). Not sure if the function > > > UmaHistogramEnumeration() works with enum classes. > > > > > > I'm fine with having the casts here and using the ENUM version of the > logging > > > macros/functions, but the special treatment of max_value made me > wonder. If > > you > > > think it's safe to reduce the upper boundary, then I'm happy to change > this > to > > > use the enum version (and add a comment in case we need the cast). > > > > > > Adding new enum values should always be ok and needs no special > treatment > > > > > > (https://codesearch.chromium.org/chromium/src/base/metrics/ > histogram_macros.h?rcl=0&l=40). > > > > Hah, from that same link: "The value in |sample| must be strictly less > than > > |enum_max|." Maybe that comment is incorrect (asvitkine should know), > but I'm > > guessing this is why the "+1" is there. > > I guess the underlying issue is that MAX_VALUE or COUNT entries in enums > are > never supposed to be used for actual values but just as sentinels. So they > should never show up in the logs. > > +Alexei, can you clarify? > > https://codereview.chromium.org/2619203007/ > -- 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.
https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:268: // If not, we could use UmaHistogramEnumeration(). It's because WindowOpenDisposition::MAX_VALUE is inclusive whereas the macros & functions expect it to be exclusive. https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics_unittest.cc (right): https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. It's 2017 now! https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics_unittest.cc:15: namespace { Nit: I don't think tests are usually put in an anon namespace.
thanks for clarifying Alexei! PTAL https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics_unittest.cc (right): https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/16 17:37:10, Alexei Svitkine (slow) wrote: > It's 2017 now! Done. Happy new year ;-) https://codereview.chromium.org/2619203007/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics_unittest.cc:15: namespace { On 2017/01/16 17:37:10, Alexei Svitkine (slow) wrote: > Nit: I don't think tests are usually put in an anon namespace. interesting. It's actually considered good practice see here https://g3doc.corp.google.com/company/readability/home/acquiring-readability/... and various threads on c-users@/c-style@. If there's something speaking against, I can also take it out.
lgtm
Marc want to have another look or should I move you to cc?
lgtm https://codereview.chromium.org/2619203007/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:268: // a valid enum value. nit: I find this sentence hard to parse :) How about: We use WindowOpenDisposition::MAX_VALUE + 1 for |value_max| since MAX_VALUE itself is still a valid enum value. https://codereview.chromium.org/2619203007/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2619203007/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38799: + units="score"> nit: It's really "score*10", right? I guess one option would be to introduce an enum in this file, to assign proper labels to each bucket.
https://codereview.chromium.org/2619203007/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2619203007/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:268: // a valid enum value. On 2017/01/17 12:13:25, Marc Treib wrote: > nit: I find this sentence hard to parse :) How about: > We use WindowOpenDisposition::MAX_VALUE + 1 for |value_max| since MAX_VALUE > itself is still a valid enum value. you're right. Used your suggestions. Thx! https://codereview.chromium.org/2619203007/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2619203007/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38799: + units="score"> On 2017/01/17 12:13:25, Marc Treib wrote: > nit: It's really "score*10", right? > I guess one option would be to introduce an enum in this file, to assign proper > labels to each bucket. cool. Gave it a shot. PTAL.
The CQ bit was checked by tschumann@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...
https://codereview.chromium.org/2619203007/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2619203007/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38799: + units="score"> On 2017/01/17 16:00:26, tschumann wrote: > On 2017/01/17 12:13:25, Marc Treib wrote: > > nit: It's really "score*10", right? > > I guess one option would be to introduce an enum in this file, to assign > proper > > labels to each bucket. > > cool. Gave it a shot. PTAL. Looks good, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tschumann@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, treib@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2619203007/#ps160001 (title: "fixed enum values")
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": 160001, "attempt_start_ts": 1484733575008280,
"parent_rev": "8f2a6f1d8aec16e71cf4fa97d5b1bae1e093d974", "commit_rev":
"ec4069a22ddb364e0f28adc4f4f5f740a477d3a0"}
Message was sent while issue was closed.
Description was changed from ========== Log suggestion scores in 10 discrete buckets. Quality scores are usually in [0,1]. As UMA histograms don't support floats, this CL changes the logging to log as integer in [1,10]. Histogram definitions cannot be changed, so this CL deprecates the old Score histograms and introduces new ScoreNormalized histograms. BUG=672525 ========== to ========== Log suggestion scores in 10 discrete buckets. Quality scores are usually in [0,1]. As UMA histograms don't support floats, this CL changes the logging to log as integer in [1,10]. Histogram definitions cannot be changed, so this CL deprecates the old Score histograms and introduces new ScoreNormalized histograms. BUG=672525 Review-Url: https://codereview.chromium.org/2619203007 Cr-Commit-Position: refs/heads/master@{#444321} Committed: https://chromium.googlesource.com/chromium/src/+/ec4069a22ddb364e0f28adc4f4f5... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/ec4069a22ddb364e0f28adc4f4f5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
