Hint about where to insert integer histogram enum values.

Hint about where to insert integer histogram enum values.

Some enums in histograms.xml are very long (e.g. LoginCustomFlags),
and inserting new values require manually finding the right spot.

This CL automates that by logging details of the existing value that
comes right before the new value.

Review-Url: https://codereview.chromium.org/2810733005
Cr-Commit-Position: refs/heads/master@{#464770}
Committed: https://chromium.googlesource.com/chromium/src/+/c8152899d3cd6c3f92446bf75bc63ce606a82bde

[DmitrySkiba, 2017-04-11 17:41:53]

dskiba@chromium.org changed reviewers:
+ isherman@chromium.org

[DmitrySkiba, 2017-04-11 17:41:53]

Example of the output:

ERROR:root:Enum LoginCustomFlags int values 2141463681 and 1176621939 are not in
numerical order
WARNING:root:Insert value 1176621939 after 1174088940 ("enable-wasm")

[Ilya Sherman, 2017-04-11 21:34:25]

Thanks for the cleanup!

https://codereview.chromium.org/2810733005/diff/1/tools/metrics/histograms/ex...
File tools/metrics/histograms/extract_histograms.py (right):

https://codereview.chromium.org/2810733005/diff/1/tools/metrics/histograms/ex...
tools/metrics/histograms/extract_histograms.py:227: return lo
Rather than copying bisect.bisect_left(), could you instead create a list that
just has the integer values, and call bisect on it directly?

https://codereview.chromium.org/2810733005/diff/1/tools/metrics/histograms/ex...
tools/metrics/histograms/extract_histograms.py:242: left_int_value,
left_value_dict = enum_items[left_item_index]
nit: It looks like the -1 calculation could be moved into this line.

[DmitrySkiba, 2017-04-12 16:24:13]

https://codereview.chromium.org/2810733005/diff/1/tools/metrics/histograms/ex...
File tools/metrics/histograms/extract_histograms.py (right):

https://codereview.chromium.org/2810733005/diff/1/tools/metrics/histograms/ex...
tools/metrics/histograms/extract_histograms.py:227: return lo
On 2017/04/11 21:34:25, Ilya Sherman wrote:
> Rather than copying bisect.bisect_left(), could you instead create a list that
> just has the integer values, and call bisect on it directly?

Done.

https://codereview.chromium.org/2810733005/diff/1/tools/metrics/histograms/ex...
tools/metrics/histograms/extract_histograms.py:242: left_int_value,
left_value_dict = enum_items[left_item_index]
On 2017/04/11 21:34:25, Ilya Sherman wrote:
> nit: It looks like the -1 calculation could be moved into this line.

Done.

[Ilya Sherman, 2017-04-12 20:29:33]

LGTM, thanks!

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

https://codereview.chromium.org/2810733005/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/extract_histograms.py:220: enum_int_values = list(i[0]
for i in enum_items)
Optional: I was thinking you could write something like

enum_int_values = sorted(enum_dict['values'].keys())

and then write line 233 as something like

left_int_value = enum_int_values[left_item_index - 1]
left_label = enum_dict['values'][left_int_value]

[DmitrySkiba, 2017-04-14 18:04:22]

On 2017/04/12 20:29:33, Ilya Sherman wrote:
> LGTM, thanks!
> 
>
https://codereview.chromium.org/2810733005/diff/20001/tools/metrics/histogram...
> File tools/metrics/histograms/extract_histograms.py (right):
> 
>
https://codereview.chromium.org/2810733005/diff/20001/tools/metrics/histogram...
> tools/metrics/histograms/extract_histograms.py:220: enum_int_values =
list(i[0]
> for i in enum_items)
> Optional: I was thinking you could write something like
> 
> enum_int_values = sorted(enum_dict['values'].keys())
> 
> and then write line 233 as something like
> 
> left_int_value = enum_int_values[left_item_index - 1]
> left_label = enum_dict['values'][left_int_value]

Yup, you are right, this is better. However, I don't have time for one more
iteration, so I'll commit it as is.

[DmitrySkiba, 2017-04-14 18:04:27]

The CQ bit was checked by dskiba@chromium.org

[commit-bot: I haz the power, 2017-04-14 18:04:48]

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-04-14 19:08:01]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492193067445660,
"parent_rev": "466bc2de9d83036de88b0eb612dbeae603fd547a", "commit_rev":
"c8152899d3cd6c3f92446bf75bc63ce606a82bde"}

[commit-bot: I haz the power, 2017-04-14 19:08:56]

Description was changed from

==========
Hint about where to insert integer histogram enum values.

Some enums in histograms.xml are very long (e.g. LoginCustomFlags),
and inserting new values require manually finding the right spot.

This CL automates that by logging details of the existing value that
comes right before the new value.
==========

to

==========
Hint about where to insert integer histogram enum values.

Some enums in histograms.xml are very long (e.g. LoginCustomFlags),
and inserting new values require manually finding the right spot.

This CL automates that by logging details of the existing value that
comes right before the new value.

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

[commit-bot: I haz the power, 2017-04-14 19:08:57]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c8152899d3cd6c3f92446bf75bc6...

[Ilya Sherman, 2017-04-14 21:28:03]

On 2017/04/14 18:04:22, DmitrySkiba wrote:
> On 2017/04/12 20:29:33, Ilya Sherman wrote:
> > LGTM, thanks!
> > 
> >
>
https://codereview.chromium.org/2810733005/diff/20001/tools/metrics/histogram...
> > File tools/metrics/histograms/extract_histograms.py (right):
> > 
> >
>
https://codereview.chromium.org/2810733005/diff/20001/tools/metrics/histogram...
> > tools/metrics/histograms/extract_histograms.py:220: enum_int_values =
> list(i[0]
> > for i in enum_items)
> > Optional: I was thinking you could write something like
> > 
> > enum_int_values = sorted(enum_dict['values'].keys())
> > 
> > and then write line 233 as something like
> > 
> > left_int_value = enum_int_values[left_item_index - 1]
> > left_label = enum_dict['values'][left_int_value]
> 
> Yup, you are right, this is better. However, I don't have time for one more
> iteration, so I'll commit it as is.

You didn't have time to tweak these lines prior to submitting the code?  I'm not
sure what that means.  I intended the LGTM to be sticky, so you would simply
have needed to make the changes in your local editor, upload a new patch set,
and then check the CQ box.

Anyway, thank you for the addition.  I sent you a small follow-up CL to address
these comments, since we both agree that the code is a bit cleaner this way:
https://codereview.chromium.org/2822783002

[DmitrySkiba, 2017-04-14 21:36:02]

On 2017/04/14 21:28:03, Ilya Sherman wrote:
> On 2017/04/14 18:04:22, DmitrySkiba wrote:
> > On 2017/04/12 20:29:33, Ilya Sherman wrote:
> > > LGTM, thanks!
> > > 
> > >
> >
>
https://codereview.chromium.org/2810733005/diff/20001/tools/metrics/histogram...
> > > File tools/metrics/histograms/extract_histograms.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2810733005/diff/20001/tools/metrics/histogram...
> > > tools/metrics/histograms/extract_histograms.py:220: enum_int_values =
> > list(i[0]
> > > for i in enum_items)
> > > Optional: I was thinking you could write something like
> > > 
> > > enum_int_values = sorted(enum_dict['values'].keys())
> > > 
> > > and then write line 233 as something like
> > > 
> > > left_int_value = enum_int_values[left_item_index - 1]
> > > left_label = enum_dict['values'][left_int_value]
> > 
> > Yup, you are right, this is better. However, I don't have time for one more
> > iteration, so I'll commit it as is.
> 
> You didn't have time to tweak these lines prior to submitting the code?  I'm
not
> sure what that means.  I intended the LGTM to be sticky, so you would simply
> have needed to make the changes in your local editor, upload a new patch set,
> and then check the CQ box.

Yes, but I literally didn't have time for that yesterday / today. Since you
marked your suggestion as "Optional", and since it's not a critical thing, I
decided to just submit CL as is and free one memory slot in my brain for other
things.

> Anyway, thank you for the addition.  I sent you a small follow-up CL to
address
> these comments, since we both agree that the code is a bit cleaner this way:
> https://codereview.chromium.org/2822783002

Thanks!