Create 'errors' histogram for failures.

Create 'errors' histogram for failures.

BUG=671649
Review-Url: https://codereview.chromium.org/2635303002
Cr-Commit-Position: refs/heads/master@{#446037}
Committed: https://chromium.googlesource.com/chromium/src/+/cc51fdc84d9c63e481dabbcdb7710d26e685940e

[bcwhite, 2017-01-17 18:56:01]

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

[commit-bot: I haz the power, 2017-01-17 18:56:37]

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

[bcwhite, 2017-01-17 20:00:11]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org

[commit-bot: I haz the power, 2017-01-17 20:03:37]

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

[commit-bot: I haz the power, 2017-01-17 20:03:38]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-01-17 20:09:27]

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

https://codereview.chromium.org/2635303002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:69832: +<histogram
name="UMA.PersistentAllocator.BrowserMetrics.Errors"
Can these definitions be re-organized using <histogram-suffixes>? Otherwise
there's a lot of duplication.

[bcwhite, 2017-01-17 20:28:49]

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

https://codereview.chromium.org/2635303002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:69832: +<histogram
name="UMA.PersistentAllocator.BrowserMetrics.Errors"
On 2017/01/17 20:09:27, Alexei Svitkine (slow) wrote:
> Can these definitions be re-organized using <histogram-suffixes>? Otherwise
> there's a lot of duplication.

Good idea.  Let me see if I can make that work.

[bcwhite, 2017-01-17 20:45:03]

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

https://codereview.chromium.org/2635303002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:69832: +<histogram
name="UMA.PersistentAllocator.BrowserMetrics.Errors"
On 2017/01/17 20:28:49, bcwhite wrote:
> On 2017/01/17 20:09:27, Alexei Svitkine (slow) wrote:
> > Can these definitions be re-organized using <histogram-suffixes>? Otherwise
> > there's a lot of duplication.
> 
> Good idea.  Let me see if I can make that work.

I don't see how.  I can't create the individual suffixes from a base name
because there is no "units=" option for the individual suffixes and all three
are different.

The "ordering=prefix" looks like the right option to create three base
histograms and insert the many allocator names... but that option only inserts
after the _first_ dot and I need it to insert after the second dot
("UMA.PersistentAllocator.NAME.whatever").

Is there another way?

[Alexei Svitkine (slow), 2017-01-17 20:49:52]

We can either switch to the new ordering for the new histograms (i.e. have
the allocator name be the suffix) or maybe better would be to just add
support for specifying where to insert the suffix (i.e. suffixdots="2" or
whatever). The processing code is own Python code in
tools/metrics/histograms so we can just modify it.

On Tue, Jan 17, 2017 at 3:45 PM, <bcwhite@chromium.org> wrote:

>
> https://codereview.chromium.org/2635303002/diff/1/tools/
> metrics/histograms/histograms.xml
> File tools/metrics/histograms/histograms.xml (right):
>
> https://codereview.chromium.org/2635303002/diff/1/tools/
> metrics/histograms/histograms.xml#newcode69832
> tools/metrics/histograms/histograms.xml:69832: +<histogram
> name="UMA.PersistentAllocator.BrowserMetrics.Errors"
> On 2017/01/17 20:28:49, bcwhite wrote:
> > On 2017/01/17 20:09:27, Alexei Svitkine (slow) wrote:
> > > Can these definitions be re-organized using <histogram-suffixes>?
> Otherwise
> > > there's a lot of duplication.
> >
> > Good idea. Let me see if I can make that work.
>
> I don't see how. I can't create the individual suffixes from a base
> name because there is no "units=" option for the individual suffixes and
> all three are different.
>
> The "ordering=prefix" looks like the right option to create three base
> histograms and insert the many allocator names... but that option only
> inserts after the _first_ dot and I need it to insert after the second
> dot ("UMA.PersistentAllocator.NAME.whatever").
>
> Is there another way?
>
> https://codereview.chromium.org/2635303002/
>

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

[Alexei Svitkine (slow), 2017-01-17 22:06:39]

Or maybe just have it be part of the order attr - e.g. ordering="prefix,2".

On Tue, Jan 17, 2017 at 3:49 PM, Alexei Svitkine <asvitkine@chromium.org>
wrote:

> We can either switch to the new ordering for the new histograms (i.e. have
> the allocator name be the suffix) or maybe better would be to just add
> support for specifying where to insert the suffix (i.e. suffixdots="2" or
> whatever). The processing code is own Python code in
> tools/metrics/histograms so we can just modify it.
>
> On Tue, Jan 17, 2017 at 3:45 PM, <bcwhite@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/2635303002/diff/1/tools/metr
>> ics/histograms/histograms.xml
>> File tools/metrics/histograms/histograms.xml (right):
>>
>> https://codereview.chromium.org/2635303002/diff/1/tools/metr
>> ics/histograms/histograms.xml#newcode69832
>> tools/metrics/histograms/histograms.xml:69832: +<histogram
>> name="UMA.PersistentAllocator.BrowserMetrics.Errors"
>> On 2017/01/17 20:28:49, bcwhite wrote:
>> > On 2017/01/17 20:09:27, Alexei Svitkine (slow) wrote:
>> > > Can these definitions be re-organized using <histogram-suffixes>?
>> Otherwise
>> > > there's a lot of duplication.
>> >
>> > Good idea. Let me see if I can make that work.
>>
>> I don't see how. I can't create the individual suffixes from a base
>> name because there is no "units=" option for the individual suffixes and
>> all three are different.
>>
>> The "ordering=prefix" looks like the right option to create three base
>> histograms and insert the many allocator names... but that option only
>> inserts after the _first_ dot and I need it to insert after the second
>> dot ("UMA.PersistentAllocator.NAME.whatever").
>>
>> Is there another way?
>>
>> https://codereview.chromium.org/2635303002/
>>
>
>

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

[bcwhite, 2017-01-18 18:16:13]

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

[commit-bot: I haz the power, 2017-01-18 18:16:45]

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

[bcwhite, 2017-01-18 18:17:42]

ordering=prefix,N it is.

I've manually tested that this change does not alter the output of
ExtractHistograms in any way.

With the new histogram_suffixes, the output changes slightly, as such:

- u'UMA.PersistentAllocator.BrowserMetrics.Allocs': {'owners':
[u'bcwhite@chromium.org',
+ u'UMA.PersistentAllocator.Allocs': {'owners': [u'bcwhite@chromium.org',
+                                                u'asvitkine@chromium.org'],
+                                     'summary': u'Size, before padding, of
objects allocated from persistent memory. This is updated with every
allocation.',
+                                     'units': u'bytes'},
+ u'UMA.PersistentAllocator.BrowserMetrics.Allocs': {'fieldtrial_groups':
[u'BrowserMetrics'],
+                                                    'fieldtrial_labels': [u'For
browser process metrics.'],
+                                                    'fieldtrial_names':
[u'PersistentMemoryAllocs'],
+                                                    'owners':
[u'bcwhite@chromium.org',
                                                               
u'asvitkine@chromium.org'],
-                                                    'summary': u'Size, before
padding, of objects allocated from persistent memory in the browser process.
This is updated with every allocation.',
+                                                    'summary': u'Size, before
padding, of objects allocated from persistent memory. This is updated with every
allocation.',
                                                     'units': u'bytes'},

The histograms have gained some "fieldtrial_foo" attributes.  Do they look
correct to you?

[Alexei Svitkine (slow), 2017-01-18 18:38:36]

I think that's correct. I think fieldtrial_* were old names for suffixes
and (histogram_suffixes used to be known as fieldtrials back when
histograms had to be named differently for field trials) - I think we have
a TODO to rename those keys.

On Wed, Jan 18, 2017 at 1:17 PM, <bcwhite@chromium.org> wrote:

> ordering=prefix,N it is.
>
> I've manually tested that this change does not alter the output of
> ExtractHistograms in any way.
>
> With the new histogram_suffixes, the output changes slightly, as such:
>
> - u'UMA.PersistentAllocator.BrowserMetrics.Allocs': {'owners':
> [u'bcwhite@chromium.org&#39;,
> + u'UMA.PersistentAllocator.Allocs': {'owners':
> [u'bcwhite@chromium.org&#39;,
> + u'asvitkine@chromium.org'],
> + 'summary': u'Size, before padding, of
> objects allocated from persistent memory. This is updated with every
> allocation.',
> + 'units': u'bytes'},
> + u'UMA.PersistentAllocator.BrowserMetrics.Allocs': {'fieldtrial_groups':
> [u'BrowserMetrics'],
> + 'fieldtrial_labels': [u'For
> browser process metrics.'],
> + 'fieldtrial_names':
> [u'PersistentMemoryAllocs'],
> + 'owners':
> [u'bcwhite@chromium.org&#39;,
>
> u'asvitkine@chromium.org'],
> - 'summary': u'Size, before
> padding, of objects allocated from persistent memory in the browser
> process.
> This is updated with every allocation.',
> + 'summary': u'Size, before
> padding, of objects allocated from persistent memory. This is updated with
> every
> allocation.',
> 'units': u'bytes'},
>
> The histograms have gained some "fieldtrial_foo" attributes. Do they look
> correct to you?
>
>
> https://codereview.chromium.org/2635303002/
>

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

[Alexei Svitkine (slow), 2017-01-18 18:46:59]

Looks great - just a few comments.

https://codereview.chromium.org/2635303002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/extract_histograms.py (right):

https://codereview.chromium.org/2635303002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/extract_histograms.py:145: parts = ordering.split(',')
Note: Please also make the same change in the internal copy of this code. Will
send you a link separately.

https://codereview.chromium.org/2635303002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/extract_histograms.py:167: 'Prefix Field Trial
expansions require histogram names which include a '
Nit: Rename this message to not say "Field Trial".

i.e. "Prefix histogram_suffixes expansions..."

also the second place on line 168.

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

https://codereview.chromium.org/2635303002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:69844: +<histogram
name="UMA.PersistentAllocator.UsedPct" units="%">
add base="true" to indicate the histogram is not logged as-is.

[bcwhite, 2017-01-18 19:05:58]

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

[commit-bot: I haz the power, 2017-01-18 19:06:36]

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

[bcwhite, 2017-01-18 19:06:59]

https://codereview.chromium.org/2635303002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/extract_histograms.py (right):

https://codereview.chromium.org/2635303002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/extract_histograms.py:145: parts = ordering.split(',')
On 2017/01/18 18:46:59, Alexei Svitkine (slow) wrote:
> Note: Please also make the same change in the internal copy of this code. Will
> send you a link separately.

Acknowledged.

https://codereview.chromium.org/2635303002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/extract_histograms.py:167: 'Prefix Field Trial
expansions require histogram names which include a '
On 2017/01/18 18:46:59, Alexei Svitkine (slow) wrote:
> Nit: Rename this message to not say "Field Trial".
> 
> i.e. "Prefix histogram_suffixes expansions..."
> 
> also the second place on line 168.

Done.

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

https://codereview.chromium.org/2635303002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:69844: +<histogram
name="UMA.PersistentAllocator.UsedPct" units="%">
On 2017/01/18 18:46:59, Alexei Svitkine (slow) wrote:
> add base="true" to indicate the histogram is not logged as-is.

Done.

[Alexei Svitkine (slow), 2017-01-18 19:10:25]

lgtm

Please don't submit until the internal changes are in and made it into
production - which should be tomorrow morning if you make those changes before
then.

[commit-bot: I haz the power, 2017-01-18 20:59:23]

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

[commit-bot: I haz the power, 2017-01-18 20:59:23]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-01-25 13:24:59]

On 2017/01/18 19:10:25, Alexei Svitkine (slow) wrote:
> lgtm
> 
> Please don't submit until the internal changes are in and made it into
> production - which should be tomorrow morning if you make those changes before
> then.

Okay to submit this now?

[bcwhite, 2017-01-25 13:25:19]

Description was changed from

==========
Create 'errors' sparse histogram for failures.

BUG=671649
==========

to

==========
Create 'errors' histogram for failures.

BUG=671649
==========

[Alexei Svitkine (slow), 2017-01-25 15:29:20]

Yes, looks like the server-side changes have been picked up.

[bcwhite, 2017-01-25 15:42:25]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2017-01-25 15:42:38]

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

[commit-bot: I haz the power, 2017-01-25 16:45:29]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485358945108770,
"parent_rev": "d927d42c252a5d2b72e73d7b514e4d6a9e0a05e1", "commit_rev":
"cc51fdc84d9c63e481dabbcdb7710d26e685940e"}

[commit-bot: I haz the power, 2017-01-25 16:46:05]

Description was changed from

==========
Create 'errors' histogram for failures.

BUG=671649
==========

to

==========
Create 'errors' histogram for failures.

BUG=671649

Review-Url: https://codereview.chromium.org/2635303002
Cr-Commit-Position: refs/heads/master@{#446037}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc51fdc84d9c63e481dabbcdb771...
==========

[commit-bot: I haz the power, 2017-01-25 16:46:06]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/cc51fdc84d9c63e481dabbcdb771...