Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1637493002:
Add SameVersionStartupCounts suffix to startup HardFault and Temperature histograms. (Closed)
DescriptionAdd SameVersionStartupCounts suffix to startup HardFault and Temperature histograms.
Also:
- Move RecordTimeSinceLastStartup() and RecordStartupCount() as implementation
details of RecordBrowserMainMessageLoopStart().
- Deprecate "Startup.BrowserMessageLoopStartHardFaultCount.FirstRun" histogram
(obsolete and also replaced by the more generic
"Startup.BrowserMessageLoopStartHardFaultCount.1")
- Fix "Startup.BrowserMessageLoopStartHardFaultCount" to use 1 instead of 0 as
minimum since 0 is implicit in UMA.
- Fix "Startup.Temperature" to use STARTUP_TEMPERATURE_MAX instead of
STARTUP_TEMPERATURE_COUNT as the |max_boundary| as this is what the
UMA_HISTOGRAM_ENUMERATION macro expects FWICT.
POST-COMMIT ERRATUM: Didn't end up doing this, see discussion at end of this code review.
BUG=580211
Committed: https://crrev.com/45a4b5a72591338c60bd57ccf58f009e07e497b0
Cr-Commit-Position: refs/heads/master@{#371630}
Patch Set 1 #
Total comments: 12
Patch Set 2 : review:fdoray #Patch Set 3 : Optional PrefService* param for html_viewer #Patch Set 4 : merge up to r371436 #
Total comments: 13
Patch Set 5 : back to _COUNT instead of _MAX #
Dependent Patchsets: Messages
Total messages: 31 (10 generated)
Description was changed from
==========
Add SameVersionStartupCounts suffix to startup HardFault and Temperature
histograms.
BUG=580211
==========
to
==========
Add SameVersionStartupCounts suffix to startup HardFault and Temperature
histograms.
Also:
- Move RecordTimeSinceLastStartup() and RecordStartupCount() as implementation
details of RecordBrowserMainMessageLoopStart().
- Deprecate "Startup.BrowserMessageLoopStartHardFaultCount.FirstRun" histogram
(obsolete and also replaced by the more generic
"Startup.BrowserMessageLoopStartHardFaultCount.1")
- Fix "Startup.BrowserMessageLoopStartHardFaultCount" to use 1 instead of 0 as
minimum since 0 is implicit in UMA.
- Fix "Startup.Temperature" to use STARTUP_TEMPERATURE_MAX instead of
STARTUP_TEMPERATURE_COUNT as the |max_boundary| as this is what the
UMA_HISTOGRAM_ENUMERATION macro expects FWICT.
BUG=580211
==========
Description was changed from
==========
Add SameVersionStartupCounts suffix to startup HardFault and Temperature
histograms.
Also:
- Move RecordTimeSinceLastStartup() and RecordStartupCount() as implementation
details of RecordBrowserMainMessageLoopStart().
- Deprecate "Startup.BrowserMessageLoopStartHardFaultCount.FirstRun" histogram
(obsolete and also replaced by the more generic
"Startup.BrowserMessageLoopStartHardFaultCount.1")
- Fix "Startup.BrowserMessageLoopStartHardFaultCount" to use 1 instead of 0 as
minimum since 0 is implicit in UMA.
- Fix "Startup.Temperature" to use STARTUP_TEMPERATURE_MAX instead of
STARTUP_TEMPERATURE_COUNT as the |max_boundary| as this is what the
UMA_HISTOGRAM_ENUMERATION macro expects FWICT.
BUG=580211
==========
to
==========
Add SameVersionStartupCounts suffix to startup HardFault and Temperature
histograms.
Also:
- Move RecordTimeSinceLastStartup() and RecordStartupCount() as implementation
details of RecordBrowserMainMessageLoopStart().
- Deprecate "Startup.BrowserMessageLoopStartHardFaultCount.FirstRun" histogram
(obsolete and also replaced by the more generic
"Startup.BrowserMessageLoopStartHardFaultCount.1")
- Fix "Startup.BrowserMessageLoopStartHardFaultCount" to use 1 instead of 0 as
minimum since 0 is implicit in UMA.
- Fix "Startup.Temperature" to use STARTUP_TEMPERATURE_MAX instead of
STARTUP_TEMPERATURE_COUNT as the |max_boundary| as this is what the
UMA_HISTOGRAM_ENUMERATION macro expects FWICT.
BUG=580211
==========
gab@chromium.org changed reviewers: + fdoray@chromium.org
Francois PTAL (+CC Chris if you feel like it too) Thanks, Gab
gab@chromium.org changed reviewers: + jwd@chromium.org, thakis@chromium.org
On 2016/01/25 18:17:42, gab wrote: > Francois PTAL (+CC Chris if you feel like it too) > > Thanks, > Gab +Nico for rubberstamp of side-effects on chrome_browser_main.cc +Jesse for histograms.xml (also PTAL at non-trivial, i.e. non-macro, histogram use-cases in startup_metric_utils.cc).
lgtm
https://codereview.chromium.org/1637493002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1637493002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:795: base::TimeTicks::Now(), is_first_run, g_browser_process->local_state()); You will need to update this call site: https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi... https://codereview.chromium.org/1637493002/diff/1/components/startup_metric_u... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1637493002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:212: const char kHardFaulCountHistogram[] = kHardFaultCount ^ https://codereview.chromium.org/1637493002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:222: ->Add(hard_fault_count); Why can't you use the UMA_HISTOGRAM_CUSTOM_COUNTS macro here? https://codereview.chromium.org/1637493002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:244: ->Add(g_startup_temperature); Why can't you use the UMA_HISTOGRAM_ENUMERATION macro here? https://codereview.chromium.org/1637493002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:537: RecordTimeSinceLastStartup(pref_service); RecordTimeSinceLastStartup needs to be called after RecordHardFaultHistogram becauses it uses the startup temperature.
@msw: see question below for html_viewer. Thanks, Gab https://codereview.chromium.org/1637493002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1637493002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:795: base::TimeTicks::Now(), is_first_run, g_browser_process->local_state()); On 2016/01/25 18:48:00, fdoray wrote: > You will need to update this call site: > https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi... Argh.. @msw: is there an equivalent to local_state in html_viewer (i.e. a persistent PrefService)? Or should we split what needs it into a separate public call and not call it from here?
msw@chromium.org changed reviewers: + msw@chromium.org
https://codereview.chromium.org/1637493002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1637493002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:795: base::TimeTicks::Now(), is_first_run, g_browser_process->local_state()); On 2016/01/25 21:20:16, gab wrote: > On 2016/01/25 18:48:00, fdoray wrote: > > You will need to update this call site: > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi... > > Argh.. @msw: is there an equivalent to local_state in html_viewer (i.e. a > persistent PrefService)? Or should we split what needs it into a separate public > call and not call it from here? No, not afaik, but you can just pass a null base::PrefService in html_viewer and handle that case within RecordBrowserMainMessageLoopStart, right?
@msw: thanks, reply inline @fdoray PTAL (will add a NULL check for html_viewer in a follow-up patch set, but gotta run now). https://codereview.chromium.org/1637493002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1637493002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:795: base::TimeTicks::Now(), is_first_run, g_browser_process->local_state()); On 2016/01/25 21:29:44, msw wrote: > On 2016/01/25 21:20:16, gab wrote: > > On 2016/01/25 18:48:00, fdoray wrote: > > > You will need to update this call site: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi... > > > > Argh.. @msw: is there an equivalent to local_state in html_viewer (i.e. a > > persistent PrefService)? Or should we split what needs it into a separate > public > > call and not call it from here? > > No, not afaik, but you can just pass a null base::PrefService in html_viewer and > handle that case within RecordBrowserMainMessageLoopStart, right? Was hoping not to add special code on our side for html_viewer, but yes.. https://codereview.chromium.org/1637493002/diff/1/components/startup_metric_u... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1637493002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:212: const char kHardFaulCountHistogram[] = On 2016/01/25 18:48:00, fdoray wrote: > kHardFaultCount > ^ Done. https://codereview.chromium.org/1637493002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:222: ->Add(hard_fault_count); On 2016/01/25 18:48:00, fdoray wrote: > Why can't you use the UMA_HISTOGRAM_CUSTOM_COUNTS macro here? Because the name is dynamic and the macros end up taking a static pointer to a single histogram which doesn't work for dynamic names : https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... Using the FactoryGet() is common for dynamically generated histogram names (i.e. to use the macro we'd need an explicit switch with 10 static names). https://codereview.chromium.org/1637493002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:244: ->Add(g_startup_temperature); On 2016/01/25 18:48:00, fdoray wrote: > Why can't you use the UMA_HISTOGRAM_ENUMERATION macro here? Same. https://codereview.chromium.org/1637493002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:537: RecordTimeSinceLastStartup(pref_service); On 2016/01/25 18:48:00, fdoray wrote: > RecordTimeSinceLastStartup needs to be called after RecordHardFaultHistogram > becauses it uses the startup temperature. Good catch :-) -- next on my list is to make that NOTREACHED() to catch those once I've addressed those that are already wrong..!
Made parameter optional to support html_viewer. @msw/fdoray PTAL Thanks, Gab
lgtm with 1 comment. https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:243: STARTUP_TEMPERATURE_MAX); The comment above the definition of UMA_HISTOGRAM_ENUMERATION says "The samples should always be strictly less than |boundary_value|". That suggests that STARTUP_TEMPERATURE_MAX should not be equal to LUKEWARM_STARTUP_TEMPERATURE. I haven't looked at the code to see if the comment is right.
Good catch, @jwd to answer question https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:243: STARTUP_TEMPERATURE_MAX); On 2016/01/26 12:13:21, fdoray wrote: > The comment above the definition of UMA_HISTOGRAM_ENUMERATION says > "The samples should always be strictly less than |boundary_value|". That > suggests that STARTUP_TEMPERATURE_MAX should not be equal to > LUKEWARM_STARTUP_TEMPERATURE. I haven't looked at the code to see if the comment > is right. Ah, I missed that comment and only looked at the code which leads to a base::LinearHistogram::FactoryGet() with |maximum| set to |boundary_value|, no where does that class say that maximum is an exclusive boundary. @jwd: what's the right thing to do here? I guess _COUNT is correct, not _MAX?
https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:243: STARTUP_TEMPERATURE_MAX); On 2016/01/26 17:59:18, gab wrote: > On 2016/01/26 12:13:21, fdoray wrote: > > The comment above the definition of UMA_HISTOGRAM_ENUMERATION says > > "The samples should always be strictly less than |boundary_value|". That > > suggests that STARTUP_TEMPERATURE_MAX should not be equal to > > LUKEWARM_STARTUP_TEMPERATURE. I haven't looked at the code to see if the > comment > > is right. > > Ah, I missed that comment and only looked at the code which leads to a > base::LinearHistogram::FactoryGet() with |maximum| set to |boundary_value|, no > where does that class say that maximum is an exclusive boundary. > > @jwd: what's the right thing to do here? I guess _COUNT is correct, not _MAX? Looking at this closer, the reason I thought this was the case is that the macro passes |boundary_value + 1| for |bucket_count|, hence I assumed it meant |boundary_value| was a valid value (though perhaps |bucket_count| includes the implicit 0?? that'd be a very confusing API if the 0 was implicit but you had to explicitly count it in your |bucket_count|...)
lgtm with nits https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.h (right): https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.h:40: UNDETERMINED_STARTUP_TEMPERATURE nit: trailing comma https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.h:81: // stats that span multiple startups; in its absence those stats will not be nit: maybe describe which stats, like the removed function comments?
@msw: replies below https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.h (right): https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.h:40: UNDETERMINED_STARTUP_TEMPERATURE On 2016/01/26 18:08:30, msw wrote: > nit: trailing comma This is on purpose, this one should always be last, new ones should go above STARTUP_TEMPERATURE_MAX. I can add a comment if you don't think this is clear. https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.h:81: // stats that span multiple startups; in its absence those stats will not be On 2016/01/26 18:08:30, msw wrote: > nit: maybe describe which stats, like the removed function comments? This is constantly subject to change so I intentionally left it vague as I think it's wrong for the API to constantly be updated when metrics are added/removed. All the caller needs to know is that : if you don't provide this param you're missing out on all stats that span multiple startups but still getting the stats specific to the current startup.
ack, lgtm https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.h (right): https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.h:40: UNDETERMINED_STARTUP_TEMPERATURE On 2016/01/26 18:15:07, gab wrote: > On 2016/01/26 18:08:30, msw wrote: > > nit: trailing comma > > This is on purpose, this one should always be last, new ones should go above > STARTUP_TEMPERATURE_MAX. I can add a comment if you don't think this is clear. Acknowledged. https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.h:81: // stats that span multiple startups; in its absence those stats will not be On 2016/01/26 18:15:07, gab wrote: > On 2016/01/26 18:08:30, msw wrote: > > nit: maybe describe which stats, like the removed function comments? > > This is constantly subject to change so I intentionally left it vague as I think > it's wrong for the API to constantly be updated when metrics are added/removed. > All the caller needs to know is that : if you don't provide this param you're > missing out on all stats that span multiple startups but still getting the stats > specific to the current startup. Acknowledged.
https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:243: STARTUP_TEMPERATURE_MAX); On 2016/01/26 17:59:18, gab wrote: > On 2016/01/26 12:13:21, fdoray wrote: > > The comment above the definition of UMA_HISTOGRAM_ENUMERATION says > > "The samples should always be strictly less than |boundary_value|". That > > suggests that STARTUP_TEMPERATURE_MAX should not be equal to > > LUKEWARM_STARTUP_TEMPERATURE. I haven't looked at the code to see if the > comment > > is right. > > Ah, I missed that comment and only looked at the code which leads to a > base::LinearHistogram::FactoryGet() with |maximum| set to |boundary_value|, no > where does that class say that maximum is an exclusive boundary. > > @jwd: what's the right thing to do here? I guess _COUNT is correct, not _MAX? fdoray is correct, the boundary value should be strictly greater than the sample values. The reason is if you ever want to add extra enum buckets. When you have a sample of |boundary_value| it's put in a bucket of range [boundary_value, max_int], which will be problematic when combining with data containing extra buckets. So strictly speaking, you can do what you're doing, but you probably shouldn't.
Thanks Jesse, updated CL accordingly, PTAL @ histograms. https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:243: STARTUP_TEMPERATURE_MAX); On 2016/01/26 18:47:18, Jesse Doherty wrote: > On 2016/01/26 17:59:18, gab wrote: > > On 2016/01/26 12:13:21, fdoray wrote: > > > The comment above the definition of UMA_HISTOGRAM_ENUMERATION says > > > "The samples should always be strictly less than |boundary_value|". That > > > suggests that STARTUP_TEMPERATURE_MAX should not be equal to > > > LUKEWARM_STARTUP_TEMPERATURE. I haven't looked at the code to see if the > > comment > > > is right. > > > > Ah, I missed that comment and only looked at the code which leads to a > > base::LinearHistogram::FactoryGet() with |maximum| set to |boundary_value|, no > > where does that class say that maximum is an exclusive boundary. > > > > @jwd: what's the right thing to do here? I guess _COUNT is correct, not _MAX? > > fdoray is correct, the boundary value should be strictly greater than the sample > values. The reason is if you ever want to add extra enum buckets. When you have > a sample of |boundary_value| it's put in a bucket of range [boundary_value, > max_int], which will be problematic when combining with data containing extra > buckets. > > So strictly speaking, you can do what you're doing, but you probably shouldn't. Aaaah got it, makes sense, thanks! (maybe the comment in histogram_macros.h could be clearer? or one on LinearHistogram that states that the max will be in such a special overflow bucket)
lgtm https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:243: STARTUP_TEMPERATURE_MAX); On 2016/01/26 20:10:22, gab wrote: > On 2016/01/26 18:47:18, Jesse Doherty wrote: > > On 2016/01/26 17:59:18, gab wrote: > > > On 2016/01/26 12:13:21, fdoray wrote: > > > > The comment above the definition of UMA_HISTOGRAM_ENUMERATION says > > > > "The samples should always be strictly less than |boundary_value|". That > > > > suggests that STARTUP_TEMPERATURE_MAX should not be equal to > > > > LUKEWARM_STARTUP_TEMPERATURE. I haven't looked at the code to see if the > > > comment > > > > is right. > > > > > > Ah, I missed that comment and only looked at the code which leads to a > > > base::LinearHistogram::FactoryGet() with |maximum| set to |boundary_value|, > no > > > where does that class say that maximum is an exclusive boundary. > > > > > > @jwd: what's the right thing to do here? I guess _COUNT is correct, not > _MAX? > > > > fdoray is correct, the boundary value should be strictly greater than the > sample > > values. The reason is if you ever want to add extra enum buckets. When you > have > > a sample of |boundary_value| it's put in a bucket of range [boundary_value, > > max_int], which will be problematic when combining with data containing extra > > buckets. > > > > So strictly speaking, you can do what you're doing, but you probably > shouldn't. > > Aaaah got it, makes sense, thanks! > > (maybe the comment in histogram_macros.h could be clearer? or one on > LinearHistogram that states that the max will be in such a special overflow > bucket) Clearer in what sense? The comment on UMA_HISTOGRAM_ENUMERATION seems unambiguous, and points to the comment on LOCAL_HISTOGRAM_ENUMERATION for a bit more information. I agree that the the comments on the FactoryGet methods are a bit unclear, but there's fairly heavy documentation at the top of histograms.h on how to use these methods.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, msw@chromium.org, fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/1637493002/#ps80001 (title: "back to _COUNT instead of _MAX")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637493002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637493002/80001
Thanks, @Jesse: reply below. https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1637493002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:243: STARTUP_TEMPERATURE_MAX); On 2016/01/26 21:45:37, Jesse Doherty wrote: > On 2016/01/26 20:10:22, gab wrote: > > On 2016/01/26 18:47:18, Jesse Doherty wrote: > > > On 2016/01/26 17:59:18, gab wrote: > > > > On 2016/01/26 12:13:21, fdoray wrote: > > > > > The comment above the definition of UMA_HISTOGRAM_ENUMERATION says > > > > > "The samples should always be strictly less than |boundary_value|". That > > > > > suggests that STARTUP_TEMPERATURE_MAX should not be equal to > > > > > LUKEWARM_STARTUP_TEMPERATURE. I haven't looked at the code to see if the > > > > comment > > > > > is right. > > > > > > > > Ah, I missed that comment and only looked at the code which leads to a > > > > base::LinearHistogram::FactoryGet() with |maximum| set to > |boundary_value|, > > no > > > > where does that class say that maximum is an exclusive boundary. > > > > > > > > @jwd: what's the right thing to do here? I guess _COUNT is correct, not > > _MAX? > > > > > > fdoray is correct, the boundary value should be strictly greater than the > > sample > > > values. The reason is if you ever want to add extra enum buckets. When you > > have > > > a sample of |boundary_value| it's put in a bucket of range [boundary_value, > > > max_int], which will be problematic when combining with data containing > extra > > > buckets. > > > > > > So strictly speaking, you can do what you're doing, but you probably > > shouldn't. > > > > Aaaah got it, makes sense, thanks! > > > > (maybe the comment in histogram_macros.h could be clearer? or one on > > LinearHistogram that states that the max will be in such a special overflow > > bucket) > > Clearer in what sense? The comment on UMA_HISTOGRAM_ENUMERATION seems > unambiguous, and points to the comment on LOCAL_HISTOGRAM_ENUMERATION for a bit > more information. I agree that the the comments on the FactoryGet methods are a > bit unclear, but there's fairly heavy documentation at the top of histograms.h > on how to use these methods. I guess I'm bad at reading comment and prefer to follow code and was expecting comments on LinearHistogram's factory methods directly. But here's a suggestion for an addition to the LOCAL_HISTOGRAM_ENUMERATION comment: "this prevents you from running into problems down the line if you add additional buckets to the histogram as the overflow bucket will be beyond your current max instead of including it" I guess no big deal now that I understand the reason, just trying to make sure no else loses time on this -- while also not wasting more time on it myself :-). Also, I'd actually read histogram.h's header comment, but couldn't find the word "enum" in it nor does it mention that the max value will be in a bucket [max, MAX_INT] and as such is bad for enums. Anyways, up to you guys if you want to do something about it, I'm just trying to explain where my misunderstanding came from in this specific case..
Message was sent while issue was closed.
Description was changed from
==========
Add SameVersionStartupCounts suffix to startup HardFault and Temperature
histograms.
Also:
- Move RecordTimeSinceLastStartup() and RecordStartupCount() as implementation
details of RecordBrowserMainMessageLoopStart().
- Deprecate "Startup.BrowserMessageLoopStartHardFaultCount.FirstRun" histogram
(obsolete and also replaced by the more generic
"Startup.BrowserMessageLoopStartHardFaultCount.1")
- Fix "Startup.BrowserMessageLoopStartHardFaultCount" to use 1 instead of 0 as
minimum since 0 is implicit in UMA.
- Fix "Startup.Temperature" to use STARTUP_TEMPERATURE_MAX instead of
STARTUP_TEMPERATURE_COUNT as the |max_boundary| as this is what the
UMA_HISTOGRAM_ENUMERATION macro expects FWICT.
BUG=580211
==========
to
==========
Add SameVersionStartupCounts suffix to startup HardFault and Temperature
histograms.
Also:
- Move RecordTimeSinceLastStartup() and RecordStartupCount() as implementation
details of RecordBrowserMainMessageLoopStart().
- Deprecate "Startup.BrowserMessageLoopStartHardFaultCount.FirstRun" histogram
(obsolete and also replaced by the more generic
"Startup.BrowserMessageLoopStartHardFaultCount.1")
- Fix "Startup.BrowserMessageLoopStartHardFaultCount" to use 1 instead of 0 as
minimum since 0 is implicit in UMA.
- Fix "Startup.Temperature" to use STARTUP_TEMPERATURE_MAX instead of
STARTUP_TEMPERATURE_COUNT as the |max_boundary| as this is what the
UMA_HISTOGRAM_ENUMERATION macro expects FWICT.
BUG=580211
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
Add SameVersionStartupCounts suffix to startup HardFault and Temperature
histograms.
Also:
- Move RecordTimeSinceLastStartup() and RecordStartupCount() as implementation
details of RecordBrowserMainMessageLoopStart().
- Deprecate "Startup.BrowserMessageLoopStartHardFaultCount.FirstRun" histogram
(obsolete and also replaced by the more generic
"Startup.BrowserMessageLoopStartHardFaultCount.1")
- Fix "Startup.BrowserMessageLoopStartHardFaultCount" to use 1 instead of 0 as
minimum since 0 is implicit in UMA.
- Fix "Startup.Temperature" to use STARTUP_TEMPERATURE_MAX instead of
STARTUP_TEMPERATURE_COUNT as the |max_boundary| as this is what the
UMA_HISTOGRAM_ENUMERATION macro expects FWICT.
BUG=580211
==========
to
==========
Add SameVersionStartupCounts suffix to startup HardFault and Temperature
histograms.
Also:
- Move RecordTimeSinceLastStartup() and RecordStartupCount() as implementation
details of RecordBrowserMainMessageLoopStart().
- Deprecate "Startup.BrowserMessageLoopStartHardFaultCount.FirstRun" histogram
(obsolete and also replaced by the more generic
"Startup.BrowserMessageLoopStartHardFaultCount.1")
- Fix "Startup.BrowserMessageLoopStartHardFaultCount" to use 1 instead of 0 as
minimum since 0 is implicit in UMA.
- Fix "Startup.Temperature" to use STARTUP_TEMPERATURE_MAX instead of
STARTUP_TEMPERATURE_COUNT as the |max_boundary| as this is what the
UMA_HISTOGRAM_ENUMERATION macro expects FWICT.
BUG=580211
Committed: https://crrev.com/45a4b5a72591338c60bd57ccf58f009e07e497b0
Cr-Commit-Position: refs/heads/master@{#371630}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/45a4b5a72591338c60bd57ccf58f009e07e497b0 Cr-Commit-Position: refs/heads/master@{#371630}
Message was sent while issue was closed.
Description was changed from
==========
Add SameVersionStartupCounts suffix to startup HardFault and Temperature
histograms.
Also:
- Move RecordTimeSinceLastStartup() and RecordStartupCount() as implementation
details of RecordBrowserMainMessageLoopStart().
- Deprecate "Startup.BrowserMessageLoopStartHardFaultCount.FirstRun" histogram
(obsolete and also replaced by the more generic
"Startup.BrowserMessageLoopStartHardFaultCount.1")
- Fix "Startup.BrowserMessageLoopStartHardFaultCount" to use 1 instead of 0 as
minimum since 0 is implicit in UMA.
- Fix "Startup.Temperature" to use STARTUP_TEMPERATURE_MAX instead of
STARTUP_TEMPERATURE_COUNT as the |max_boundary| as this is what the
UMA_HISTOGRAM_ENUMERATION macro expects FWICT.
BUG=580211
Committed: https://crrev.com/45a4b5a72591338c60bd57ccf58f009e07e497b0
Cr-Commit-Position: refs/heads/master@{#371630}
==========
to
==========
Add SameVersionStartupCounts suffix to startup HardFault and Temperature
histograms.
Also:
- Move RecordTimeSinceLastStartup() and RecordStartupCount() as implementation
details of RecordBrowserMainMessageLoopStart().
- Deprecate "Startup.BrowserMessageLoopStartHardFaultCount.FirstRun" histogram
(obsolete and also replaced by the more generic
"Startup.BrowserMessageLoopStartHardFaultCount.1")
- Fix "Startup.BrowserMessageLoopStartHardFaultCount" to use 1 instead of 0 as
minimum since 0 is implicit in UMA.
- Fix "Startup.Temperature" to use STARTUP_TEMPERATURE_MAX instead of
STARTUP_TEMPERATURE_COUNT as the |max_boundary| as this is what the
UMA_HISTOGRAM_ENUMERATION macro expects FWICT.
POST-COMMIT ERRATUM: Didn't end up doing this, see discussion at end of this
code review.
BUG=580211
Committed: https://crrev.com/45a4b5a72591338c60bd57ccf58f009e07e497b0
Cr-Commit-Position: refs/heads/master@{#371630}
==========
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
