Add SameVersionStartupCounts suffix to multiple startup histograms.

Add SameVersionStartupCounts suffix to multiple startup histograms.

With this CL, the following histograms are recorded with a suffix
indicating the number of startups with the same version:
- Startup.BrowserMainToRendererMain
- Startup.BrowserMessageLoopStartTime
- Startup.BrowserMessageLoopStartTimeFromMainEntry
- Startup.BrowserOpenTabs
- Startup.BrowserWindowDisplay
- Startup.FirstWebContents.MainFrameLoad2
- Startup.FirstWebContents.MainNavigationFinished
- Startup.FirstWebContents.MainNavigationStart
- Startup.FirstWebContents.NonEmptyPaint2
- Startup.LoadTime.ExeMainToDllMain
- Startup.LoadTime.ProcessCreateToDllMain
- Startup.LoadTime.ProcessCreateToExeMain
- Startup.SystemUptime
- Startup.TimeSinceLastStartup

BUG=580211
Committed: https://crrev.com/f1cf57529db3bc83795ae34db80c8c1742420aeb
Cr-Commit-Position: refs/heads/master@{#406046}

[fdoray, 2016-07-05 17:01:03]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org

[fdoray, 2016-07-05 17:01:04]

Can you review this CL? Thanks.

[gab, 2016-07-07 15:41:56]

overall lg, thanks

Add to CL description that this will not be appended to the temperature suffix
but rather be reported side-by-side it.

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:156: /* Record
to an histogram suffixed with the number of startups for the    \
*a* histogram (below as well)

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:163:
type(basename + same_version_startup_count_suffix, value);              \
Since |type| is a macro and is thus not guaranteed to be a single statement (it
is in practice but doesn't have to be), I think it's better to use {} even for
"single-line body" in this case?

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:183:
kUndeterminedStartupsWithCurrentVersion) {
When would this be empty? When |pref_service| is nullptr in
RecordBrowserMainMessageLoopStart? FWICT this is always called with
|local_state()| so perhaps it should never be null? Should we try to remove that
condition and DCHECK(pref_service) in there instead in a precursor CL?

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:191: const int
kMaxSameVersionCountRecorded = 9;
constexpr

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:247:
UMA_HISTOGRAM_CUSTOM_COUNTS(
Add a comment that this method should only be called once and
|same_version_startup_count_suffix| should be constant in this process (DCHECK
!empty on it above to confirm?) so using the static macro is fine.

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

https://codereview.chromium.org/2117373003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:99420: +     
name="Startup.BrowserMessageLoopStartTimeFromMainEntry.FirstRun"/>
It's overkill for the first run histogram.

Cold/warm probably also is FWIW (we could look at stats but I'd be very
surprised to see many warm first runs; it's possible with --first-run but very
unlikely in the wild). So maybe not suffixing that one is the right thing to do.

https://codereview.chromium.org/2117373003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:99422: +  <affected-histogram
name="Startup.BrowserWindowDisplay"/>
 - RecordBrowserWindowDisplay()
 - RecordBrowserOpenTabsDelta()

are called too early for warm/cold today and may also be for this:
http://crbug.com/580207

We don't have to fix this in this CL but those suffixes are likely broken until
we do.

I think what we should do is have a method which histogram logging is delegated
to and which knows to queue them until all suffixes have been registered so that
the early ones all get properly suffixed.

https://codereview.chromium.org/2117373003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:99422: +  <affected-histogram
name="Startup.BrowserWindowDisplay"/>
Also note that BrowserWindowDisplay is itself broken in its own ways:
http://crbug.com/589118

[fdoray, 2016-07-08 14:52:52]

PTAnL

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:156: /* Record
to an histogram suffixed with the number of startups for the    \
On 2016/07/07 15:41:56, gab wrote:
> *a* histogram (below as well)

Done.

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:163:
type(basename + same_version_startup_count_suffix, value);              \
On 2016/07/07 15:41:55, gab wrote:
> Since |type| is a macro and is thus not guaranteed to be a single statement
(it
> is in practice but doesn't have to be), I think it's better to use {} even for
> "single-line body" in this case?

Done.

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:183:
kUndeterminedStartupsWithCurrentVersion) {
On 2016/07/07 15:41:56, gab wrote:
> When would this be empty? When |pref_service| is nullptr in
> RecordBrowserMainMessageLoopStart? FWICT this is always called with
> |local_state()| so perhaps it should never be null? Should we try to remove
that
> condition and DCHECK(pref_service) in there instead in a precursor CL?

DCHECK(pref_service) in a separate CL: Done.

g_startups_with_current_version == kUndeterminedStartupsWithCurrentVersion can
still happen because of crbug.com/580207 I'll try to understand this bug.

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:191: const int
kMaxSameVersionCountRecorded = 9;
On 2016/07/07 15:41:55, gab wrote:
> constexpr

Done.

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:247:
UMA_HISTOGRAM_CUSTOM_COUNTS(
On 2016/07/07 15:41:56, gab wrote:
> Add a comment that this method should only be called once and
> |same_version_startup_count_suffix| should be constant in this process (DCHECK
> !empty on it above to confirm?) so using the static macro is fine.

Done.

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

https://codereview.chromium.org/2117373003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:99420: +     
name="Startup.BrowserMessageLoopStartTimeFromMainEntry.FirstRun"/>
On 2016/07/07 15:41:56, gab wrote:
> It's overkill for the first run histogram.
> 
> Cold/warm probably also is FWIW (we could look at stats but I'd be very
> surprised to see many warm first runs; it's possible with --first-run but very
> unlikely in the wild). So maybe not suffixing that one is the right thing to
do.

Done.

https://codereview.chromium.org/2117373003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:99422: +  <affected-histogram
name="Startup.BrowserWindowDisplay"/>
On 2016/07/07 15:41:56, gab wrote:
>  - RecordBrowserWindowDisplay()
>  - RecordBrowserOpenTabsDelta()
> 
> are called too early for warm/cold today and may also be for this:
> http://crbug.com/580207
> 
> We don't have to fix this in this CL but those suffixes are likely broken
until
> we do.
> 
> I think what we should do is have a method which histogram logging is
delegated
> to and which knows to queue them until all suffixes have been registered so
that
> the early ones all get properly suffixed.

ok, I keep these histograms here and we'll fix them in separate CLs?

[fdoray, 2016-07-08 14:53:01]

Description was changed from

==========
Add SameVersionStartupCounts suffix to multiple startup histograms.

With this CL, the following histograms are recorded with a suffix
indicating the number of startups with the same version:
- Startup.BrowserMainToRendererMain
- Startup.BrowserMessageLoopStartTime
- Startup.BrowserMessageLoopStartTimeFromMainEntry
- Startup.BrowserMessageLoopStartTimeFromMainEntry.FirstRun
- Startup.BrowserOpenTabs
- Startup.BrowserWindowDisplay
- Startup.FirstWebContents.MainFrameLoad2
- Startup.FirstWebContents.MainNavigationFinished
- Startup.FirstWebContents.MainNavigationStart
- Startup.FirstWebContents.NonEmptyPaint2
- Startup.LoadTime.ExeMainToDllMain
- Startup.LoadTime.ProcessCreateToDllMain
- Startup.LoadTime.ProcessCreateToExeMain
- Startup.SystemUptime
- Startup.TimeSinceLastStartup

BUG=580211
==========

to

==========
Add SameVersionStartupCounts suffix to multiple startup histograms.

With this CL, the following histograms are recorded with a suffix
indicating the number of startups with the same version:
- Startup.BrowserMainToRendererMain
- Startup.BrowserMessageLoopStartTime
- Startup.BrowserMessageLoopStartTimeFromMainEntry
- Startup.BrowserOpenTabs
- Startup.BrowserWindowDisplay
- Startup.FirstWebContents.MainFrameLoad2
- Startup.FirstWebContents.MainNavigationFinished
- Startup.FirstWebContents.MainNavigationStart
- Startup.FirstWebContents.NonEmptyPaint2
- Startup.LoadTime.ExeMainToDllMain
- Startup.LoadTime.ProcessCreateToDllMain
- Startup.LoadTime.ProcessCreateToExeMain
- Startup.SystemUptime
- Startup.TimeSinceLastStartup

BUG=580211
==========

[fdoray, 2016-07-08 15:17:09]

fdoray@chromium.org changed reviewers:
+ primiano@chromium.org, rkaplow@chromium.org

[fdoray, 2016-07-08 15:17:10]

+rkaplow@: Can you review tools/metrics/histograms/histograms.xml?

+primiano@: Can you review base/trace_event/common/trace_event_common.h?

Thanks!

[rkaplow, 2016-07-08 17:16:18]

https://codereview.chromium.org/2117373003/diff/80001/components/startup_metr...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2117373003/diff/80001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:275:
UMA_HISTOGRAM_CUSTOM_COUNTS(
I would leave the previous version - for these macros it should be a constant
string as an argumment.

https://codereview.chromium.org/2117373003/diff/80001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:298:
UMA_HISTOGRAM_ENUMERATION(
here too

[gab, 2016-07-08 19:56:19]

lgtm

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2117373003/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:183:
kUndeterminedStartupsWithCurrentVersion) {
On 2016/07/08 14:52:51, fdoray wrote:
> On 2016/07/07 15:41:56, gab wrote:
> > When would this be empty? When |pref_service| is nullptr in
> > RecordBrowserMainMessageLoopStart? FWICT this is always called with
> > |local_state()| so perhaps it should never be null? Should we try to remove
> that
> > condition and DCHECK(pref_service) in there instead in a precursor CL?
> 
> DCHECK(pref_service) in a separate CL: Done.
> 
> g_startups_with_current_version == kUndeterminedStartupsWithCurrentVersion can
> still happen because of crbug.com/580207 I'll try to understand this bug.

Perfect, thanks, feel free to take ownership of both bugs.

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

https://codereview.chromium.org/2117373003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:99422: +  <affected-histogram
name="Startup.BrowserWindowDisplay"/>
On 2016/07/08 14:52:52, fdoray wrote:
> On 2016/07/07 15:41:56, gab wrote:
> >  - RecordBrowserWindowDisplay()
> >  - RecordBrowserOpenTabsDelta()
> > 
> > are called too early for warm/cold today and may also be for this:
> > http://crbug.com/580207
> > 
> > We don't have to fix this in this CL but those suffixes are likely broken
> until
> > we do.
> > 
> > I think what we should do is have a method which histogram logging is
> delegated
> > to and which knows to queue them until all suffixes have been registered so
> that
> > the early ones all get properly suffixed.
> 
> ok, I keep these histograms here and we'll fix them in separate CLs?

sgtm

[fdoray, 2016-07-08 20:24:34]

PTAnL

https://codereview.chromium.org/2117373003/diff/80001/components/startup_metr...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2117373003/diff/80001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:275:
UMA_HISTOGRAM_CUSTOM_COUNTS(
On 2016/07/08 17:16:17, rkaplow wrote:
> I would leave the previous version - for these macros it should be a constant
> string as an argumment.

Done.

https://codereview.chromium.org/2117373003/diff/80001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:298:
UMA_HISTOGRAM_ENUMERATION(
On 2016/07/08 17:16:18, rkaplow wrote:
> here too

Done.

[rkaplow, 2016-07-11 15:41:51]

lgtm

[gab, 2016-07-11 18:09:16]

re-lgtm % nit

https://codereview.chromium.org/2117373003/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (left):

https://codereview.chromium.org/2117373003/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:223: // Factory
properties copied from UMA_HISTOGRAM_CUSTOM_COUNTS macro.
Leave this comment in since still using FactoryGet()

https://codereview.chromium.org/2117373003/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:246: // Factory
properties copied from UMA_HISTOGRAM_ENUMERATION macro.
Ditto

[Primiano Tucci (use gerrit), 2016-07-12 14:53:22]

Oops somehow lost track of this. 
//base/trace_event LGTM

[fdoray, 2016-07-18 14:54:39]

https://codereview.chromium.org/2117373003/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (left):

https://codereview.chromium.org/2117373003/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:223: // Factory
properties copied from UMA_HISTOGRAM_CUSTOM_COUNTS macro.
On 2016/07/11 18:09:16, gab -- OOO until Monday wrote:
> Leave this comment in since still using FactoryGet() 

Done.

https://codereview.chromium.org/2117373003/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:246: // Factory
properties copied from UMA_HISTOGRAM_ENUMERATION macro.
On 2016/07/11 18:09:16, gab -- OOO until Monday wrote:
> Ditto

Done.

[fdoray, 2016-07-18 14:54:48]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-07-18 14:54:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, gab@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2117373003/#ps120001
(title: "CR gab #12 (restore comments)")

[commit-bot: I haz the power, 2016-07-18 14:55:29]

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, 2016-07-18 17:30:23]

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

[commit-bot: I haz the power, 2016-07-18 17:30:24]

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_...)

[fdoray, 2016-07-18 17:40:04]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-07-18 17:40:42]

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, 2016-07-18 18:41:43]

Description was changed from

==========
Add SameVersionStartupCounts suffix to multiple startup histograms.

With this CL, the following histograms are recorded with a suffix
indicating the number of startups with the same version:
- Startup.BrowserMainToRendererMain
- Startup.BrowserMessageLoopStartTime
- Startup.BrowserMessageLoopStartTimeFromMainEntry
- Startup.BrowserOpenTabs
- Startup.BrowserWindowDisplay
- Startup.FirstWebContents.MainFrameLoad2
- Startup.FirstWebContents.MainNavigationFinished
- Startup.FirstWebContents.MainNavigationStart
- Startup.FirstWebContents.NonEmptyPaint2
- Startup.LoadTime.ExeMainToDllMain
- Startup.LoadTime.ProcessCreateToDllMain
- Startup.LoadTime.ProcessCreateToExeMain
- Startup.SystemUptime
- Startup.TimeSinceLastStartup

BUG=580211
==========

to

==========
Add SameVersionStartupCounts suffix to multiple startup histograms.

With this CL, the following histograms are recorded with a suffix
indicating the number of startups with the same version:
- Startup.BrowserMainToRendererMain
- Startup.BrowserMessageLoopStartTime
- Startup.BrowserMessageLoopStartTimeFromMainEntry
- Startup.BrowserOpenTabs
- Startup.BrowserWindowDisplay
- Startup.FirstWebContents.MainFrameLoad2
- Startup.FirstWebContents.MainNavigationFinished
- Startup.FirstWebContents.MainNavigationStart
- Startup.FirstWebContents.NonEmptyPaint2
- Startup.LoadTime.ExeMainToDllMain
- Startup.LoadTime.ProcessCreateToDllMain
- Startup.LoadTime.ProcessCreateToExeMain
- Startup.SystemUptime
- Startup.TimeSinceLastStartup

BUG=580211
==========

[commit-bot: I haz the power, 2016-07-18 18:41:45]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-18 18:41:47]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 18:43:16]

Description was changed from

==========
Add SameVersionStartupCounts suffix to multiple startup histograms.

With this CL, the following histograms are recorded with a suffix
indicating the number of startups with the same version:
- Startup.BrowserMainToRendererMain
- Startup.BrowserMessageLoopStartTime
- Startup.BrowserMessageLoopStartTimeFromMainEntry
- Startup.BrowserOpenTabs
- Startup.BrowserWindowDisplay
- Startup.FirstWebContents.MainFrameLoad2
- Startup.FirstWebContents.MainNavigationFinished
- Startup.FirstWebContents.MainNavigationStart
- Startup.FirstWebContents.NonEmptyPaint2
- Startup.LoadTime.ExeMainToDllMain
- Startup.LoadTime.ProcessCreateToDllMain
- Startup.LoadTime.ProcessCreateToExeMain
- Startup.SystemUptime
- Startup.TimeSinceLastStartup

BUG=580211
==========

to

==========
Add SameVersionStartupCounts suffix to multiple startup histograms.

With this CL, the following histograms are recorded with a suffix
indicating the number of startups with the same version:
- Startup.BrowserMainToRendererMain
- Startup.BrowserMessageLoopStartTime
- Startup.BrowserMessageLoopStartTimeFromMainEntry
- Startup.BrowserOpenTabs
- Startup.BrowserWindowDisplay
- Startup.FirstWebContents.MainFrameLoad2
- Startup.FirstWebContents.MainNavigationFinished
- Startup.FirstWebContents.MainNavigationStart
- Startup.FirstWebContents.NonEmptyPaint2
- Startup.LoadTime.ExeMainToDllMain
- Startup.LoadTime.ProcessCreateToDllMain
- Startup.LoadTime.ProcessCreateToExeMain
- Startup.SystemUptime
- Startup.TimeSinceLastStartup

BUG=580211

Committed: https://crrev.com/f1cf57529db3bc83795ae34db80c8c1742420aeb
Cr-Commit-Position: refs/heads/master@{#406046}
==========

[commit-bot: I haz the power, 2016-07-18 18:43:18]

Patchset 7 (id:??) landed as
https://crrev.com/f1cf57529db3bc83795ae34db80c8c1742420aeb
Cr-Commit-Position: refs/heads/master@{#406046}