Issue 344883002: Collect UMA statistics on which chrome://flags lead to chrome restart on ChromeOS.

Alexander Alekseev

Please review: sky@ : chrome/browser/about_flags.cc chrome/browser/about_flags.h chrome/browser/about_flags_unittest.cc dpolukhin@, nkostylev@ : all

6 years, 6 months ago (2014-06-19 16:15:51 UTC) #1

Nikita (slow)

https://codereview.chromium.org/344883002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/1/chrome/browser/about_flags.cc#newcode2160 chrome/browser/about_flags.cc:2160: std::equal(new_flags.begin(), new_flags.end(), active_flags.begin()); result = https://codereview.chromium.org/344883002/diff/1/chrome/browser/about_flags.h File chrome/browser/about_flags.h (right): ...

6 years, 6 months ago (2014-06-19 17:08:08 UTC) #2

Nikita (slow)

https://codereview.chromium.org/344883002/diff/1/chrome/browser/chromeos/login/report_restarting_flags.cc File chrome/browser/chromeos/login/report_restarting_flags.cc (right): https://codereview.chromium.org/344883002/diff/1/chrome/browser/chromeos/login/report_restarting_flags.cc#newcode296 chrome/browser/chromeos/login/report_restarting_flags.cc:296: LOG(ERROR) << "ReportRestartingFlags(): i=" << i << " \"" ...

6 years, 6 months ago (2014-06-19 17:26:45 UTC) #3

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/1/chrome/browser/about_flags.cc#newcode2160 chrome/browser/about_flags.cc:2160: std::equal(new_flags.begin(), new_flags.end(), active_flags.begin()); On 2014/06/19 17:08:07, Nikita Kostylev wrote: ...

6 years, 6 months ago (2014-06-19 18:17:47 UTC) #4

sky

https://codereview.chromium.org/344883002/diff/20001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/20001/chrome/browser/about_flags.cc#newcode1940 chrome/browser/about_flags.cc:1940: // chrome/browser/chromeos/login/report_custom_flags.cc This is not maintainable. You need to ...

6 years, 6 months ago (2014-06-19 20:23:28 UTC) #6

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/20001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/20001/chrome/browser/about_flags.cc#newcode1940 chrome/browser/about_flags.cc:1940: // chrome/browser/chromeos/login/report_custom_flags.cc On 2014/06/19 20:23:28, sky wrote: > This ...

6 years, 6 months ago (2014-06-20 18:42:45 UTC) #7

chromium-reviews

One possibility would be to use a sparse histogram and log the hash of the ...

6 years, 6 months ago (2014-06-20 18:49:26 UTC) #8

One possibility would be to use a sparse histogram and log the hash of the
command-line flag as the value. This way, you don't have to worry about
preserving an arbitrary order and you don't need the table.

However, you'll still need to have a mechanism to update histograms.xml
with the mapping between hashes and corresponding strings. So probably
still a script that parses chrome_switches.cc? And probably run this script
on presubmit if the switches file is changed? I guess it's a bit tricky if
we consider the scenario where some switches may be defined elsewhere (e.g.
in components, or something else).

On Fri, Jun 20, 2014 at 2:42 PM, <alemate@chromium.org> wrote:

>
> https://codereview.chromium.org/344883002/diff/20001/
> chrome/browser/about_flags.cc
> File chrome/browser/about_flags.cc (right):
>
> https://codereview.chromium.org/344883002/diff/20001/
> chrome/browser/about_flags.cc#newcode1940
> chrome/browser/about_flags.cc:1940: //
> chrome/browser/chromeos/login/report_custom_flags.cc
> On 2014/06/19 20:23:28, sky wrote:
>
>> This is not maintainable. You need to come up with a better way to
>>
> make this
>
>> maintainable. Maybe by exposing some way to get the set of switches?
>>
> Hopefully
>
>> in some way that doesn't require you to have a stable index.
>>
>
> We need histogram here to know which flags lead to bad user experience
> most. So we need a stable index here. And even more, it must be in
> histograms.xml .
>
> Right now it is designed in a way, that new flags which were not added
> to report_custom_flags.cc, will be reported as "unknown flag".
> And if we see large value in "unknown flag" bucket, we can add these
> flags manually.
>
> On the other hand, I can create script like extract_actions.py to
> rebuild that table and histograms.xml. And another script to presubmit
> to check for consistency. But it will not handle merge automatically. It
> is also problematic to automatically analyze all #ifdefs (like #ifndef
> OS_ANDROID).
>
>
> So, what design would you prefer?
>
> https://codereview.chromium.org/344883002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Alexander Alekseev

On 2014/06/20 18:49:26, chromium-reviews wrote: > One possibility would be to use a sparse histogram ...

6 years, 6 months ago (2014-06-20 18:55:37 UTC) #9

On 2014/06/20 18:49:26, chromium-reviews wrote:
> One possibility would be to use a sparse histogram and log the hash of the
> command-line flag as the value. This way, you don't have to worry about
> preserving an arbitrary order and you don't need the table.
> 
> However, you'll still need to have a mechanism to update histograms.xml
> with the mapping between hashes and corresponding strings. So probably
> still a script that parses chrome_switches.cc? And probably run this script
> on presubmit if the switches file is changed? I guess it's a bit tricky if
> we consider the scenario where some switches may be defined elsewhere (e.g.
> in components, or something else).

To calculate hashes in presubmit hook, we actually need to run preprocessor on
chrome_switches.cc in the mode of "Chrome for ChromeOS".

Also, as far as I know, we now have another shell, so there are already 2
compile
targets "chrome for chromeOS". One including ash/switches and another excluding
;-)

So I think that making table with integer indexes is even.


> On Fri, Jun 20, 2014 at 2:42 PM, <mailto:alemate@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/344883002/diff/20001/
> > chrome/browser/about_flags.cc
> > File chrome/browser/about_flags.cc (right):
> >
> > https://codereview.chromium.org/344883002/diff/20001/
> > chrome/browser/about_flags.cc#newcode1940
> > chrome/browser/about_flags.cc:1940: //
> > chrome/browser/chromeos/login/report_custom_flags.cc
> > On 2014/06/19 20:23:28, sky wrote:
> >
> >> This is not maintainable. You need to come up with a better way to
> >>
> > make this
> >
> >> maintainable. Maybe by exposing some way to get the set of switches?
> >>
> > Hopefully
> >
> >> in some way that doesn't require you to have a stable index.
> >>
> >
> > We need histogram here to know which flags lead to bad user experience
> > most. So we need a stable index here. And even more, it must be in
> > histograms.xml .
> >
> > Right now it is designed in a way, that new flags which were not added
> > to report_custom_flags.cc, will be reported as "unknown flag".
> > And if we see large value in "unknown flag" bucket, we can add these
> > flags manually.
> >
> > On the other hand, I can create script like extract_actions.py to
> > rebuild that table and histograms.xml. And another script to presubmit
> > to check for consistency. But it will not handle merge automatically. It
> > is also problematic to automatically analyze all #ifdefs (like #ifndef
> > OS_ANDROID).
> >
> >
> > So, what design would you prefer?
> >
> > https://codereview.chromium.org/344883002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

chromium-reviews

Can you ignore the preprocessor and generate hashes for every entry you see? Having extra ...

6 years, 6 months ago (2014-06-20 19:00:11 UTC) #10

Can you ignore the preprocessor and generate hashes for every entry you
see? Having extra stuff in histograms.xml isn't a problem...
On 20 Jun 2014 14:55, <alemate@chromium.org> wrote:

> On 2014/06/20 18:49:26, chromium-reviews wrote:
>
>> One possibility would be to use a sparse histogram and log the hash of the
>> command-line flag as the value. This way, you don't have to worry about
>> preserving an arbitrary order and you don't need the table.
>>
>
>  However, you'll still need to have a mechanism to update histograms.xml
>> with the mapping between hashes and corresponding strings. So probably
>> still a script that parses chrome_switches.cc? And probably run this
>> script
>> on presubmit if the switches file is changed? I guess it's a bit tricky if
>> we consider the scenario where some switches may be defined elsewhere
>> (e.g.
>> in components, or something else).
>>
>
> To calculate hashes in presubmit hook, we actually need to run
> preprocessor on
> chrome_switches.cc in the mode of "Chrome for ChromeOS".
>
> Also, as far as I know, we now have another shell, so there are already 2
> compile
> targets "chrome for chromeOS". One including ash/switches and another
> excluding
> ;-)
>
> So I think that making table with integer indexes is even.
>
>
>  On Fri, Jun 20, 2014 at 2:42 PM, <mailto:alemate@chromium.org> wrote:
>>
>
>  >
>> > https://codereview.chromium.org/344883002/diff/20001/
>> > chrome/browser/about_flags.cc
>> > File chrome/browser/about_flags.cc (right):
>> >
>> > https://codereview.chromium.org/344883002/diff/20001/
>> > chrome/browser/about_flags.cc#newcode1940
>> > chrome/browser/about_flags.cc:1940: //
>> > chrome/browser/chromeos/login/report_custom_flags.cc
>> > On 2014/06/19 20:23:28, sky wrote:
>> >
>> >> This is not maintainable. You need to come up with a better way to
>> >>
>> > make this
>> >
>> >> maintainable. Maybe by exposing some way to get the set of switches?
>> >>
>> > Hopefully
>> >
>> >> in some way that doesn't require you to have a stable index.
>> >>
>> >
>> > We need histogram here to know which flags lead to bad user experience
>> > most. So we need a stable index here. And even more, it must be in
>> > histograms.xml .
>> >
>> > Right now it is designed in a way, that new flags which were not added
>> > to report_custom_flags.cc, will be reported as "unknown flag".
>> > And if we see large value in "unknown flag" bucket, we can add these
>> > flags manually.
>> >
>> > On the other hand, I can create script like extract_actions.py to
>> > rebuild that table and histograms.xml. And another script to presubmit
>> > to check for consistency. But it will not handle merge automatically. It
>> > is also problematic to automatically analyze all #ifdefs (like #ifndef
>> > OS_ANDROID).
>> >
>> >
>> > So, what design would you prefer?
>> >
>> > https://codereview.chromium.org/344883002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/344883002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Alexander Alekseev

On 2014/06/20 19:00:11, chromium-reviews wrote: > Can you ignore the preprocessor and generate hashes for ...

6 years, 6 months ago (2014-06-20 19:14:27 UTC) #11

chromium-reviews

It's possible to get pretty good results with a simple Python parser. We do similar ...

6 years, 6 months ago (2014-06-20 19:19:29 UTC) #12

Alexander Alekseev

On 2014/06/20 19:19:29, chromium-reviews wrote: > It's possible to get pretty good results with a ...

6 years, 6 months ago (2014-06-20 19:27:07 UTC) #13

sky

On Fri, Jun 20, 2014 at 12:27 PM, <alemate@chromium.org> wrote: > On 2014/06/20 19:19:29, chromium-reviews ...

6 years, 6 months ago (2014-06-20 22:53:01 UTC) #14

On Fri, Jun 20, 2014 at 12:27 PM, <alemate@chromium.org> wrote:

> On 2014/06/20 19:19:29, chromium-reviews wrote:
>
>> It's possible to get pretty good results with a simple Python parser. We
>> do
>> similar things in other places too. I suspect that because the
>> chrome_switches.cc code is pretty uniform, that you should be able to get
>> away with doing that without needing to do full compilation.
>>
>
>  (Anyway, it's up to you which approach to take, I'm merely making a
>> suggestion.)
>>
>
>
> Actually I think that creating parsers, presubmit hooks and other
> automation is trading support complexity of
>     const char* const chrome_custom_histogram_switches[]
> for support complexity of presubmit hooks.
>
> The problem is that manual support of chrome_custom_histogram_switches[]
> affect only maintainers of chrome/browser/chromeos/login/* , but
> presubmit hooks will affect all chrome commits.
>

The problem is we have a comment like:

// Add new choice before this line.
//
// If it can be used on ChromeOS, please also add it to
// chrome/browser/chromeos/login/report_custom_flags.cc

at the bottom of an approximately 2000 line switch statement. Very few
people are going to see it, and then they're going to have to figure out
what report_custom_flags.cc is. Worse yet, if I remove a switch I have to
look at report_custom_flags.cc and attempt to figure it out, which again,
it's easy to miss a comment at the top of ~300 line list. This is painful,
tedious and error prone.

So, there needs to be an automated way for you to maintain what you want.
One thought is add an integer to Experiment. Yes, developers would have to
add it, but I think we can deal with it. You could add a unit test to
ensure all values are unique.

  -Scott

> But sky@ has a lot more experience in that, so I hope to get a piece of
> advice
> here.
>
>
> https://codereview.chromium.org/344883002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Alexander Alekseev

On 2014/06/20 22:53:01, sky wrote: > On Fri, Jun 20, 2014 at 12:27 PM, <mailto:alemate@chromium.org> ...

6 years, 6 months ago (2014-06-20 23:15:27 UTC) #15

Alexander Alekseev

I've moved switches histogram IDs to about_flags. I'll add test coverage for: bool AreSwitchesIdenticalToCurrentCommandLine( const ...

6 years, 6 months ago (2014-06-24 20:56:42 UTC) #17

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/1/chrome/browser/chromeos/login/report_restarting_flags.cc File chrome/browser/chromeos/login/report_restarting_flags.cc (right): https://codereview.chromium.org/344883002/diff/1/chrome/browser/chromeos/login/report_restarting_flags.cc#newcode329 chrome/browser/chromeos/login/report_restarting_flags.cc:329: UMA_HISTOGRAM_SPARSE_SLOWLY("Login.RestartingFlags", uma_id); On 2014/06/19 17:26:44, Nikita Kostylev wrote: > ...

6 years, 6 months ago (2014-06-24 21:51:53 UTC) #18

sky

I'm a bit swamped at the moment with reviews and demo stuff. I'm swapping myself ...

6 years, 6 months ago (2014-06-25 16:14:36 UTC) #20

Lei Zhang

If you go with the manually curated enum list, consider writing a presubmit check that ...

6 years, 6 months ago (2014-06-25 23:52:16 UTC) #24

Alexander Alekseev

On 2014/06/25 23:52:16, Lei Zhang wrote: > If you go with the manually curated enum ...

6 years, 6 months ago (2014-06-26 00:10:23 UTC) #25

Lei Zhang

On 2014/06/26 00:10:23, alemate wrote: > On 2014/06/25 23:52:16, Lei Zhang wrote: > > If ...

6 years, 6 months ago (2014-06-26 01:11:16 UTC) #26

Lei Zhang

Some other thoughts: alemate: Regarding this feature in general: How often do CrOS users mess ...

6 years, 6 months ago (2014-06-26 01:17:44 UTC) #27

Alexander Alekseev

On 2014/06/26 01:17:44, Lei Zhang wrote: > Some other thoughts: > > alemate: Regarding this ...

6 years, 6 months ago (2014-06-26 01:54:50 UTC) #28

Lei Zhang

On 2014/06/26 01:54:50, alemate wrote: > On 2014/06/26 01:17:44, Lei Zhang wrote: > > Some ...

6 years, 6 months ago (2014-06-26 05:25:30 UTC) #29

Alexander Alekseev

On 2014/06/26 05:25:30, Lei Zhang wrote: > On 2014/06/26 01:54:50, alemate wrote: > > On ...

6 years, 6 months ago (2014-06-26 14:13:04 UTC) #30

Nikita (slow)

On 2014/06/26 14:13:04, alemate wrote: > On 2014/06/26 05:25:30, Lei Zhang wrote: > > On ...

6 years, 6 months ago (2014-06-26 14:56:27 UTC) #31

Lei Zhang

On 2014/06/26 14:56:27, Nikita Kostylev wrote: > On 2014/06/26 14:13:04, alemate wrote: > > On ...

6 years, 6 months ago (2014-06-26 16:42:43 UTC) #32

On 2014/06/26 14:56:27, Nikita Kostylev wrote:
> On 2014/06/26 14:13:04, alemate wrote:
> > On 2014/06/26 05:25:30, Lei Zhang wrote:
> > > On 2014/06/26 01:54:50, alemate wrote:
> > > > On 2014/06/26 01:17:44, Lei Zhang wrote:
> > > > > Some other thoughts:
> > > > > 
> > > > > alemate: Regarding this feature in general: How often do CrOS users
mess
> > > with
> > > > > about:flags in the first place?
> > > > 
> > > > This is the big question we want to know: what is the real impact of
flags
> > on
> > > > [login] user experience on ChromeOS?
> > > > On the one hand, it's sometimes very painfull (up to 40 seconds "black
> > screen"
> > > > in some cases).
> > > > On the other, it is pretty difficult to fix the problem "once and for
all"
> > > > (probably it would never be totally fixed).
> > > > So we are analyzing the impact.
> > > 
> > > So there's actually already UMA data recorded about flags. See
> > > RecordUMAStatistics() in about_flags.cc. Can you record your data in that
> way
> > to
> > > avoid the need to maintain the enum values?
> > 
> > RecordUMAStatistics() doesn't build histogram.
> > So we do not know the relative impact of flags.
> > To build histogram we need to add numerical IDs.
> > To build IDs we need to keep the list somewhere.
> > That is why we need enums and all around.
> 
> Btw, I've looked up AboutFlags* at https://uma.googleplex.com/p/chrome/counts/
> For Chrome OS it has some strange data (as if flags are almost not used at
all).
> Either not reported properly or something else is broken.
> 
> AboutFlags_StartupTick	1723463	
> 24.3% (367777 users)
> 
> AboutFlags_enable-nacl	570	 
> 0.01% (146 users)
> 
> AboutFlags_enable-smooth-scrolling	423	
> 0.01% (132 users)
> 
> AboutFlags_extension-apis	392
> 0.01% (124 users)
> 
> AboutFlags_enable-pnacl	260	
> 0.00% (64 users)
> 
> AboutFlags_enable-nacl-debug	153	 
> 0.00% (28 users)
> 
> AboutFlags_print-preview	43	
> 0.00% (16 users)
> 
> AboutFlags_enable-nacl-exception-handling	2	 
> 0.00% (2 users)

Yes, the data does look odd, and if we care, we probably should investigate that
and fix it. But given the numbers for StartupTick, AboutFlags_StartupTick, and
AboutFlags_foo_flags, isn't that enough data to build a histogram without using
UMA_HISTOGRAM and needing enums?

Alexander Alekseev

On 2014/06/26 16:42:43, Lei Zhang wrote: > On 2014/06/26 14:56:27, Nikita Kostylev wrote: > > ...

6 years, 6 months ago (2014-06-26 17:32:41 UTC) #33

On 2014/06/26 16:42:43, Lei Zhang wrote:
> On 2014/06/26 14:56:27, Nikita Kostylev wrote:
> > On 2014/06/26 14:13:04, alemate wrote:
> > > On 2014/06/26 05:25:30, Lei Zhang wrote:
> > > > On 2014/06/26 01:54:50, alemate wrote:
> > > > > On 2014/06/26 01:17:44, Lei Zhang wrote:
> > > > > > Some other thoughts:
> > > > > > 
> > > > > > alemate: Regarding this feature in general: How often do CrOS users
> mess
> > > > with
> > > > > > about:flags in the first place?
> > > > > 
> > > > > This is the big question we want to know: what is the real impact of
> flags
> > > on
> > > > > [login] user experience on ChromeOS?
> > > > > On the one hand, it's sometimes very painfull (up to 40 seconds "black
> > > screen"
> > > > > in some cases).
> > > > > On the other, it is pretty difficult to fix the problem "once and for
> all"
> > > > > (probably it would never be totally fixed).
> > > > > So we are analyzing the impact.
> > > > 
> > > > So there's actually already UMA data recorded about flags. See
> > > > RecordUMAStatistics() in about_flags.cc. Can you record your data in
that
> > way
> > > to
> > > > avoid the need to maintain the enum values?
> > > 
> > > RecordUMAStatistics() doesn't build histogram.
> > > So we do not know the relative impact of flags.
> > > To build histogram we need to add numerical IDs.
> > > To build IDs we need to keep the list somewhere.
> > > That is why we need enums and all around.
> > 
> > Btw, I've looked up AboutFlags* at
https://uma.googleplex.com/p/chrome/counts/
> > For Chrome OS it has some strange data (as if flags are almost not used at
> all).
> > Either not reported properly or something else is broken.
> > 
...
> Yes, the data does look odd, and if we care, we probably should investigate
that
> and fix it. But given the numbers for StartupTick, AboutFlags_StartupTick, and
> AboutFlags_foo_flags, isn't that enough data to build a histogram without
using
> UMA_HISTOGRAM and needing enums?

Do you mean "build a histogram" on a server side?
Then we would need this registry on a server side.

chromium-reviews

We don't need the registry server-side, since it can be the equivalent of a sparse ...

6 years, 6 months ago (2014-06-26 17:52:28 UTC) #34

We don't need the registry server-side, since it can be the equivalent of a
sparse histogram server-side via the name hashes. I think it should be
possible to do, I can assist you with the server-side code on this.


On Thu, Jun 26, 2014 at 1:32 PM, <alemate@chromium.org> wrote:

> On 2014/06/26 16:42:43, Lei Zhang wrote:
>
>> On 2014/06/26 14:56:27, Nikita Kostylev wrote:
>> > On 2014/06/26 14:13:04, alemate wrote:
>> > > On 2014/06/26 05:25:30, Lei Zhang wrote:
>> > > > On 2014/06/26 01:54:50, alemate wrote:
>> > > > > On 2014/06/26 01:17:44, Lei Zhang wrote:
>> > > > > > Some other thoughts:
>> > > > > >
>> > > > > > alemate: Regarding this feature in general: How often do CrOS
>> users
>> mess
>> > > > with
>> > > > > > about:flags in the first place?
>> > > > >
>> > > > > This is the big question we want to know: what is the real impact
>> of
>> flags
>> > > on
>> > > > > [login] user experience on ChromeOS?
>> > > > > On the one hand, it's sometimes very painfull (up to 40 seconds
>> "black
>> > > screen"
>> > > > > in some cases).
>> > > > > On the other, it is pretty difficult to fix the problem "once and
>> for
>> all"
>> > > > > (probably it would never be totally fixed).
>> > > > > So we are analyzing the impact.
>> > > >
>> > > > So there's actually already UMA data recorded about flags. See
>> > > > RecordUMAStatistics() in about_flags.cc. Can you record your data in
>>
> that
>
>> > way
>> > > to
>> > > > avoid the need to maintain the enum values?
>> > >
>> > > RecordUMAStatistics() doesn't build histogram.
>> > > So we do not know the relative impact of flags.
>> > > To build histogram we need to add numerical IDs.
>> > > To build IDs we need to keep the list somewhere.
>> > > That is why we need enums and all around.
>> >
>> > Btw, I've looked up AboutFlags* at
>>
> https://uma.googleplex.com/p/chrome/counts/
>
>> > For Chrome OS it has some strange data (as if flags are almost not used
>> at
>> all).
>> > Either not reported properly or something else is broken.
>> >
>>
> ...
>
>  Yes, the data does look odd, and if we care, we probably should
>> investigate
>>
> that
>
>> and fix it. But given the numbers for StartupTick,
>> AboutFlags_StartupTick, and
>> AboutFlags_foo_flags, isn't that enough data to build a histogram without
>>
> using
>
>> UMA_HISTOGRAM and needing enums?
>>
>
> Do you mean "build a histogram" on a server side?
> Then we would need this registry on a server side.
>
>
>
> https://codereview.chromium.org/344883002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Lei Zhang

I chatted with Alexander but he still likes the UMA_HISTOGRAM as is better than using ...

6 years, 6 months ago (2014-06-26 20:03:14 UTC) #35

Alexander Alekseev

Please review. Additional reviewer: jochen@ : content/public/common/content_switches.* https://codereview.chromium.org/344883002/diff/60001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/60001/chrome/browser/about_flags.cc#newcode152 chrome/browser/about_flags.cc:152: {IDS_GENERIC_EXPERIMENT_CHOICE_DEFAULT, "", ...

6 years, 5 months ago (2014-06-27 19:28:37 UTC) #36

Alexander Alekseev

On 2014/06/27 19:28:37, alemate wrote: > Please review. > > Additional reviewer: > jochen@ : ...

6 years, 5 months ago (2014-06-27 19:31:40 UTC) #37

Lei Zhang

Did you want to add yourself as an owner for about_flags_switches_histogram_ids.h? https://codereview.chromium.org/344883002/diff/80001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): ...

6 years, 5 months ago (2014-06-27 19:46:45 UTC) #38

Alexander Alekseev

+jochen Additional reviewer: jochen@ : content/public/common/content_switches.* jochen@ : I found one android-only command-line switch, which ...

6 years, 5 months ago (2014-06-30 15:06:02 UTC) #39

jochen (gone - plz use gerrit)

where is the switch used? If it's not used in content, it shouldn't be in ...

6 years, 5 months ago (2014-06-30 15:14:39 UTC) #40

Alexander Alekseev

On 2014/06/30 15:14:39, jochen wrote: > where is the switch used? If it's not used ...

6 years, 5 months ago (2014-06-30 15:20:14 UTC) #41

jochen (gone - plz use gerrit)

according to crbug.com/356775 this flag should just be removed

6 years, 5 months ago (2014-07-01 08:24:52 UTC) #42

Alexander Alekseev

On 2014/07/01 08:24:52, jochen wrote: > according to crbug.com/356775 this flag should just be removed ...

6 years, 5 months ago (2014-07-01 12:23:50 UTC) #43

Alexander Alekseev

On 2014/06/27 19:46:45, Lei Zhang wrote: > Did you want to add yourself as an ...

6 years, 5 months ago (2014-07-01 13:34:21 UTC) #44

jochen (gone - plz use gerrit)

+dmazzoni - can you remove the now obsolete from from crbug.com/356775 so we don't have ...

6 years, 5 months ago (2014-07-02 13:22:16 UTC) #45

dmazzoni

Sure, you can remove the native android accessibility flag now - or feel free to ...

6 years, 5 months ago (2014-07-02 13:33:45 UTC) #46

Alexander Alekseev

On 2014/07/02 13:33:45, dmazzoni wrote: > Sure, you can remove the native android accessibility flag ...

6 years, 5 months ago (2014-07-02 13:45:38 UTC) #47

Alexander Alekseev

I've removed "enable-accessibility-script-injection" from about_flags.cc

6 years, 5 months ago (2014-07-02 14:01:08 UTC) #48

Lei Zhang

lgtm Drop the unused UMA_HISTOGRAM_ID_kEnableAccessibilityScriptInjection ?

6 years, 5 months ago (2014-07-02 17:42:46 UTC) #50

Alexei Svitkine (slow)

https://codereview.chromium.org/344883002/diff/140001/chrome/browser/chromeos/login/login_utils.cc File chrome/browser/chromeos/login/login_utils.cc (right): https://codereview.chromium.org/344883002/diff/140001/chrome/browser/chromeos/login/login_utils.cc#newcode127 chrome/browser/chromeos/login/login_utils.cc:127: UMA_HISTOGRAM_SPARSE_SLOWLY("Login.CustomFlags", uma_id); Wouldn't this histogram be useful on other ...

6 years, 5 months ago (2014-07-02 17:45:41 UTC) #51

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/140001/chrome/browser/chromeos/login/login_utils.cc File chrome/browser/chromeos/login/login_utils.cc (right): https://codereview.chromium.org/344883002/diff/140001/chrome/browser/chromeos/login/login_utils.cc#newcode127 chrome/browser/chromeos/login/login_utils.cc:127: UMA_HISTOGRAM_SPARSE_SLOWLY("Login.CustomFlags", uma_id); On 2014/07/02 17:45:40, Alexei Svitkine wrote: > ...

6 years, 5 months ago (2014-07-02 17:55:04 UTC) #52

Alexander Alekseev

On 2014/07/02 17:42:46, Lei Zhang wrote: > lgtm > > Drop the unused UMA_HISTOGRAM_ID_kEnableAccessibilityScriptInjection ? ...

6 years, 5 months ago (2014-07-02 18:00:25 UTC) #53

Lei Zhang

https://codereview.chromium.org/344883002/diff/140001/chrome/browser/chromeos/login/login_utils.cc File chrome/browser/chromeos/login/login_utils.cc (right): https://codereview.chromium.org/344883002/diff/140001/chrome/browser/chromeos/login/login_utils.cc#newcode127 chrome/browser/chromeos/login/login_utils.cc:127: UMA_HISTOGRAM_SPARSE_SLOWLY("Login.CustomFlags", uma_id); On 2014/07/02 17:55:03, alemate wrote: > On ...

6 years, 5 months ago (2014-07-02 19:01:05 UTC) #54

Alexander Alekseev

On 2014/07/02 19:01:05, Lei Zhang wrote: > https://codereview.chromium.org/344883002/diff/140001/chrome/browser/chromeos/login/login_utils.cc > File chrome/browser/chromeos/login/login_utils.cc (right): > > https://codereview.chromium.org/344883002/diff/140001/chrome/browser/chromeos/login/login_utils.cc#newcode127 ...

6 years, 5 months ago (2014-07-02 20:39:27 UTC) #55

Lei Zhang

On 2014/07/02 17:45:41, Alexei Svitkine wrote: > https://codereview.chromium.org/344883002/diff/140001/chrome/browser/chromeos/login/login_utils.cc > File chrome/browser/chromeos/login/login_utils.cc (right): > > https://codereview.chromium.org/344883002/diff/140001/chrome/browser/chromeos/login/login_utils.cc#newcode127 ...

6 years, 5 months ago (2014-07-02 20:48:58 UTC) #56

Alexander Alekseev

isherman@, please review: tools/metrics/histograms/histograms.xml chrome/browser/about_flags_unittest.cc (because about_flags_unittest.cc is now cheking histograms.xml contents)

6 years, 5 months ago (2014-07-03 14:02:52 UTC) #58

Ilya Sherman

You may wish to crib more from [ https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser/extension_function_histogram_value.h ]. That design seems to work ...

6 years, 5 months ago (2014-07-08 04:22:11 UTC) #59

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags.h File chrome/browser/about_flags.h (right): https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags.h#newcode68 chrome/browser/about_flags.h:68: const int command_line_switch_histogram_id; On 2014/07/08 04:22:11, Ilya Sherman wrote: ...

6 years, 5 months ago (2014-07-08 20:45:37 UTC) #60

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
chrome/browser/about_flags.h:68: const int command_line_switch_histogram_id;
On 2014/07/08 04:22:11, Ilya Sherman wrote:
> Why is this of type int, rather than of type SwitchesUmaHistogramIds (which,
> incidentally, should probably be given a singular name rather than a plural
> one)?

Done.

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
File chrome/browser/about_flags_switches_histogram_ids.h (right):

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
chrome/browser/about_flags_switches_histogram_ids.h:17: switch was removed.
On 2014/07/08 04:22:11, Ilya Sherman wrote:
> nit: The more standard practice is to prepend "DEPRECATED_" to the enum
> constant.

Done.

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
chrome/browser/about_flags_unittest.cc:862:
histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml"));
On 2014/07/08 04:22:11, Ilya Sherman wrote:
> It seems more appropriate to use a presubmit check, rather than reading and
> parsing histograms.xml via a test.

This has been discussed here (and in private talk).

presubmit check might be tricky here:

1) There is no list of all command-line switches . So one of the ideas discussed
was to grep all constant strings from all source files  like *switches*.cc . But
it is going to be very unstable. There are also cases like 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/abo...

2) A lot of flags are under different #ifdefs, so you actually need to compile
code to parse the list in a stable way.

What this CL does:
1) It introduces full list of command-line switches used in about_flags.cc .
This list is now manually defined and fixed.

2) It actually verifies that compiled code matches histogram.xml.
That is why I've put all checks to test.

3) Unlike presubmit check, this CL cannot involve any damage to commits besides
files in CL. So it is less dangerous.

That is why I've added histograms.xml parsing.

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

https://codereview.chromium.org/344883002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9525: +    in.
On 2014/07/08 04:22:11, Ilya Sherman wrote:
> Why log these only on ChromeOS, rather than on all platforms?

Because ChromeOS is now the only OS, where this enum is used.
I hope that the description will be updated, if it starts to be used on other
OS.

https://codereview.chromium.org/344883002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:39285: +  <int value="0"
label="UNKNOWN_FLAG">Unknown flag.</int>
On 2014/07/08 04:22:11, Ilya Sherman wrote:
> nit: I'd abbreviate this to <int value="0" label="Unknown flag"/>

The idea is to have here an identifier, that can be searched in source code.
I also hope the most often used descriptions to be updated later by the users of
histogram.

https://codereview.chromium.org/344883002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:39333: +  </int>
On 2014/07/08 04:22:11, Ilya Sherman wrote:
> There is really no reason not to write this as
> 
>   <int value="20"  label="file-manager-enable-new-audio-player"/>

Done.

https://codereview.chromium.org/344883002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:39832: +  <int value="243"
label="disable-click-delay">disable-click-delay</int>
On 2014/07/08 04:22:11, Ilya Sherman wrote:
> Please include a script to autogenerated this histogram, as is done for other
> large and frequently updated histograms.

As there is no list of switch values in about_flags.cc, I see the only source to
generate this list could be the source of unit test.

So I decided that it would be simplier (and error-prone) not write this script
for now.

Ilya Sherman

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags_unittest.cc File chrome/browser/about_flags_unittest.cc (right): https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags_unittest.cc#newcode862 chrome/browser/about_flags_unittest.cc:862: histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml")); On 2014/07/08 20:45:36, alemate wrote: > On 2014/07/08 ...

6 years, 5 months ago (2014-07-09 00:10:51 UTC) #61

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
chrome/browser/about_flags_unittest.cc:862:
histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml"));
On 2014/07/08 20:45:36, alemate wrote:
> On 2014/07/08 04:22:11, Ilya Sherman wrote:
> > It seems more appropriate to use a presubmit check, rather than reading and
> > parsing histograms.xml via a test.
> 
> This has been discussed here (and in private talk).
> 
> presubmit check might be tricky here:
> 
> 1) There is no list of all command-line switches . So one of the ideas
discussed
> was to grep all constant strings from all source files  like *switches*.cc .
But
> it is going to be very unstable. There are also cases like 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/abo...
> 
> 2) A lot of flags are under different #ifdefs, so you actually need to compile
> code to parse the list in a stable way.
> 
> 
> What this CL does:
> 1) It introduces full list of command-line switches used in about_flags.cc .
> This list is now manually defined and fixed.

This list needs to be updated every time a new commandline switch is added. 
That's pretty painful.

> 2) It actually verifies that compiled code matches histogram.xml.
> That is why I've put all checks to test.

The enum doesn't need to be compiled -- it has the same values on all platforms,
and is easy to grep through.

> 3) Unlike presubmit check, this CL cannot involve any damage to commits
besides
> files in CL. So it is less dangerous.

I don't understand this comment.

> That is why I've added histograms.xml parsing.

In addition to my concerns above, I find this test code far too complex for a
unit test.  I can't just look at the test and quickly grok what it's doing.  If
the test were to fail, it wouldn't be obvious to me whether that's caused by a
bug in my code, or a bug in the test code.

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

https://codereview.chromium.org/344883002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9525: +    in.
On 2014/07/08 20:45:36, alemate wrote:
> On 2014/07/08 04:22:11, Ilya Sherman wrote:
> > Why log these only on ChromeOS, rather than on all platforms?
> 
> Because ChromeOS is now the only OS, where this enum is used.
> I hope that the description will be updated, if it starts to be used on other
> OS.

You are currently adding code that only emits data on ChromeOS.  My question is:
Why only emit on ChromeOS?  Why not emit this data on all platforms?

https://codereview.chromium.org/344883002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:39832: +  <int value="243"
label="disable-click-delay">disable-click-delay</int>
On 2014/07/08 20:45:37, alemate wrote:
> On 2014/07/08 04:22:11, Ilya Sherman wrote:
> > Please include a script to autogenerated this histogram, as is done for
other
> > large and frequently updated histograms.
> 
> As there is no list of switch values in about_flags.cc, I see the only source
to
> generate this list could be the source of unit test.
> 
> So I decided that it would be simplier (and error-prone) not write this script
> for now.

There's a list in about_flags_switches_histogram_id.h (which, btw, I would still
name about_flags_switches_histogram_ids.h, despite the singular enum name).

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags_unittest.cc File chrome/browser/about_flags_unittest.cc (right): https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags_unittest.cc#newcode862 chrome/browser/about_flags_unittest.cc:862: histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml")); On 2014/07/09 00:10:50, Ilya Sherman wrote: > On ...

6 years, 5 months ago (2014-07-11 18:13:27 UTC) #62

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
chrome/browser/about_flags_unittest.cc:862:
histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml"));
On 2014/07/09 00:10:50, Ilya Sherman wrote:
> On 2014/07/08 20:45:36, alemate wrote:
> > On 2014/07/08 04:22:11, Ilya Sherman wrote:
> > > It seems more appropriate to use a presubmit check, rather than reading
and
> > > parsing histograms.xml via a test.
> > 
> > This has been discussed here (and in private talk).
> > 
> > presubmit check might be tricky here:
> > 
> > 1) There is no list of all command-line switches . So one of the ideas
> discussed
> > was to grep all constant strings from all source files  like *switches*.cc .
> But
> > it is going to be very unstable. There are also cases like 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/abo...
> > 
> > 2) A lot of flags are under different #ifdefs, so you actually need to
compile
> > code to parse the list in a stable way.
> > 
> > 
> > What this CL does:
> > 1) It introduces full list of command-line switches used in about_flags.cc .
> > This list is now manually defined and fixed.
> 
> This list needs to be updated every time a new commandline switch is added. 
> That's pretty painful.

You mean "every time a new commandline switch is added to
about_flags" it has to be added to two other files. We do not have to
do it for all flags.

> > 2) It actually verifies that compiled code matches histogram.xml.
> > That is why I've put all checks to test.
> 
> The enum doesn't need to be compiled -- it has the same values on all
platforms,
> and is easy to grep through.

But enum doesn't have string representation of values. Adding switch
names as values seems more readable.

> > 3) Unlike presubmit check, this CL cannot involve any damage to commits
> besides
> > files in CL. So it is less dangerous.
> 
> I don't understand this comment.

Presubmit checks run on each developer workstation on each commit.
If there is an error in presubmit code or environment, it can damage
the commit itself.

> > That is why I've added histograms.xml parsing.
> 
> In addition to my concerns above, I find this test code far too
> complex for a unit test.  I can't just look at the test and quickly
> grok what it's doing.  If the test were to fail, it wouldn't be
> obvious to me whether that's caused by a bug in my code, or a bug
> in the test code.

Probably I should add more comments on what's happening here.

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

https://codereview.chromium.org/344883002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9525: +    in.
On 2014/07/09 00:10:50, Ilya Sherman wrote:
> On 2014/07/08 20:45:36, alemate wrote:
> > On 2014/07/08 04:22:11, Ilya Sherman wrote:
> > > Why log these only on ChromeOS, rather than on all platforms?
> > 
> > Because ChromeOS is now the only OS, where this enum is used.
> > I hope that the description will be updated, if it starts to be used on
other
> > OS.
> 
> You are currently adding code that only emits data on ChromeOS.  My question
is:
> Why only emit on ChromeOS?
> Why not emit this data on all platforms?

The data I report here, exists on ChromeOS only.

The histogram implementation, can report any set of about_flags.
But the actual set named "Login.CustomFlags" exists on ChromeOS only. It is
symmetric difference between a set of device owner (plus policy) flags and
current user flags. This difference leads to chrome process restart on user
login.

As you can see in comments (thestig@ and asvitkine@), the histogram code might
be used to report other sets of flags, but I don't know what they are planning
to report. So I expect future users to update the description (of enum below).

https://codereview.chromium.org/344883002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:39832: +  <int value="243"
label="disable-click-delay">disable-click-delay</int>
On 2014/07/09 00:10:50, Ilya Sherman wrote:
> On 2014/07/08 20:45:37, alemate wrote:
> > On 2014/07/08 04:22:11, Ilya Sherman wrote:
> > > Please include a script to autogenerated this histogram, as is done for
> other
> > > large and frequently updated histograms.
> > 
> > As there is no list of switch values in about_flags.cc, I see the only
source
> to
> > generate this list could be the source of unit test.
> > 
> > So I decided that it would be simplier (and error-prone) not write this
script
> > for now.
> 
> There's a list in about_flags_switches_histogram_id.h (which, btw, I would
still
> name about_flags_switches_histogram_ids.h, despite the singular enum name).

I've renamed the file.

Ilya Sherman

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags_unittest.cc File chrome/browser/about_flags_unittest.cc (right): https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags_unittest.cc#newcode862 chrome/browser/about_flags_unittest.cc:862: histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml")); On 2014/07/11 18:13:27, alemate wrote: > On 2014/07/09 ...

6 years, 5 months ago (2014-07-12 00:25:10 UTC) #63

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
chrome/browser/about_flags_unittest.cc:862:
histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml"));
On 2014/07/11 18:13:27, alemate wrote:
> On 2014/07/09 00:10:50, Ilya Sherman wrote:
> > On 2014/07/08 20:45:36, alemate wrote:
> > > On 2014/07/08 04:22:11, Ilya Sherman wrote:
> > > > It seems more appropriate to use a presubmit check, rather than reading
> and
> > > > parsing histograms.xml via a test.
> > > 
> > > This has been discussed here (and in private talk).
> > > 
> > > presubmit check might be tricky here:
> > > 
> > > 1) There is no list of all command-line switches . So one of the ideas
> > discussed
> > > was to grep all constant strings from all source files  like *switches*.cc
.
> > But
> > > it is going to be very unstable. There are also cases like 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/abo...
> > > 
> > > 2) A lot of flags are under different #ifdefs, so you actually need to
> compile
> > > code to parse the list in a stable way.
> > > 
> > > 
> > > What this CL does:
> > > 1) It introduces full list of command-line switches used in about_flags.cc
.
> > > This list is now manually defined and fixed.
> > 
> > This list needs to be updated every time a new commandline switch is added. 
> > That's pretty painful.
> 
> You mean "every time a new commandline switch is added to
> about_flags" it has to be added to two other files. We do not have to
> do it for all flags.

Correct, but new flags are expected to be added to about_flags, so the
difference is largely academic.

> > > 2) It actually verifies that compiled code matches histogram.xml.
> > > That is why I've put all checks to test.
> > 
> > The enum doesn't need to be compiled -- it has the same values on all
> platforms,
> > and is easy to grep through.
> 
> But enum doesn't have string representation of values. Adding switch
> names as values seems more readable.

It looks like trimming UMA_HISTOGRAM_ID_ from the enum entry name gives the
switch name.

> > > 3) Unlike presubmit check, this CL cannot involve any damage to commits
> > besides
> > > files in CL. So it is less dangerous.
> > 
> > I don't understand this comment.
> 
> Presubmit checks run on each developer workstation on each commit.
> If there is an error in presubmit code or environment, it can damage
> the commit itself.

I still don't understand.  Is this a general argument that all presubmit checks
should be removed and replaced with tests?  The presubmit check that I'm
recommending is a read-only check, so I don't understand in what way it might
damage a commit.

Ilya Sherman

https://codereview.chromium.org/344883002/diff/220001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/220001/chrome/browser/about_flags.cc#newcode2603 chrome/browser/about_flags.cc:2603: // skip '--' before switch name nit: Comments should ...

6 years, 5 months ago (2014-07-12 00:30:56 UTC) #64

Alexander Alekseev

I've added presubmit hook + test for it. jochen@, please review python code. about_flags_switches_histogram_ids_checker_test.py is ...

6 years, 5 months ago (2014-07-21 15:22:28 UTC) #65

I've added presubmit hook + test for it.

jochen@, please review python code.

about_flags_switches_histogram_ids_checker_test.py is nearly a copy of
strict_enum_value_checker_test.py .

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
chrome/browser/about_flags_unittest.cc:862:
histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml"));
On 2014/07/12 00:25:10, Ilya Sherman wrote:
> On 2014/07/11 18:13:27, alemate wrote:
> > On 2014/07/09 00:10:50, Ilya Sherman wrote:
> > > On 2014/07/08 20:45:36, alemate wrote:
> > > > On 2014/07/08 04:22:11, Ilya Sherman wrote:
> > > > > 2) It actually verifies that compiled code matches histogram.xml.
> > > > > That is why I've put all checks to test.
> > > 
> > > The enum doesn't need to be compiled -- it has the same values on all
> > platforms,
> > > and is easy to grep through.
> > 
> > But enum doesn't have string representation of values. Adding switch
> > names as values seems more readable.
> 
> It looks like trimming UMA_HISTOGRAM_ID_ from the enum entry name gives the
> switch name.
> 
> > > > 3) Unlike presubmit check, this CL cannot involve any damage to commits
> > > besides
> > > > files in CL. So it is less dangerous.
> > > 
> > > I don't understand this comment.
> > 
> > Presubmit checks run on each developer workstation on each commit.
> > If there is an error in presubmit code or environment, it can damage
> > the commit itself.
> 
> I still don't understand.  Is this a general argument that all presubmit
checks
> should be removed and replaced with tests?  The presubmit check that I'm
> recommending is a read-only check, so I don't understand in what way it might
> damage a commit.

I think that presubmit checks verify the submitted change itself (like style,
attributes, permissions, etc...). They cannot check the build result and should
not pretend they can. The functionality should be checked by tests. On the other
hand, tests cannot check code style, so presubmit hooks are handy.

I've added presubmit hook to check header style.

https://codereview.chromium.org/344883002/diff/220001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/344883002/diff/220001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2603: // skip '--' before switch name
On 2014/07/12 00:30:56, Ilya Sherman wrote:
> nit: Comments should be complete sentences, so start with an uppercase letter
> and end with a period.

Done.

https://codereview.chromium.org/344883002/diff/220001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2618: << switch_name << "') is unknown.";
On 2014/07/12 00:30:56, Ilya Sherman wrote:
> Can this just be a NOTREACHED()?

Done.

https://codereview.chromium.org/344883002/diff/220001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2623: << "' has incorrect format.";
On 2014/07/12 00:30:56, Ilya Sherman wrote:
> Can this just be a NOTREACHED()?

Done.

https://codereview.chromium.org/344883002/diff/220001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2626: << *it << "', uma_id=" << uma_id;
On 2014/07/12 00:30:56, Ilya Sherman wrote:
> This seems redundant with the view presented in about:histograms.

ReportCustomFlags is used here to report the difference between two sets of
flags.

Also, about:histograms is available only until chrome restart.
But on ChromeOS restart could happen while there is no access to
about:histograms.

https://codereview.chromium.org/344883002/diff/220001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2627:
UMA_HISTOGRAM_SPARSE_SLOWLY(uma_histogram_hame, uma_id);
On 2014/07/12 00:30:56, Ilya Sherman wrote:
> When using UMA_HISTOGRAM_FOO macros, the histogram name must be a runtime
> constant.  Hence, you should not pass it in as a parameter.

Done.

Ilya Sherman

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags_unittest.cc File chrome/browser/about_flags_unittest.cc (right): https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags_unittest.cc#newcode862 chrome/browser/about_flags_unittest.cc:862: histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml")); On 2014/07/21 15:22:27, alemate wrote: > On 2014/07/12 ...

6 years, 5 months ago (2014-07-21 22:13:00 UTC) #66

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
chrome/browser/about_flags_unittest.cc:862:
histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml"));
On 2014/07/21 15:22:27, alemate wrote:
> On 2014/07/12 00:25:10, Ilya Sherman wrote:
> > On 2014/07/11 18:13:27, alemate wrote:
> > > On 2014/07/09 00:10:50, Ilya Sherman wrote:
> > > > On 2014/07/08 20:45:36, alemate wrote:
> > > > > On 2014/07/08 04:22:11, Ilya Sherman wrote:
> > > > > > 2) It actually verifies that compiled code matches histogram.xml.
> > > > > > That is why I've put all checks to test.
> > > > 
> > > > The enum doesn't need to be compiled -- it has the same values on all
> > > platforms,
> > > > and is easy to grep through.
> > > 
> > > But enum doesn't have string representation of values. Adding switch
> > > names as values seems more readable.
> > 
> > It looks like trimming UMA_HISTOGRAM_ID_ from the enum entry name gives the
> > switch name.
> > 
> > > > > 3) Unlike presubmit check, this CL cannot involve any damage to
commits
> > > > besides
> > > > > files in CL. So it is less dangerous.
> > > > 
> > > > I don't understand this comment.
> > > 
> > > Presubmit checks run on each developer workstation on each commit.
> > > If there is an error in presubmit code or environment, it can damage
> > > the commit itself.
> > 
> > I still don't understand.  Is this a general argument that all presubmit
> checks
> > should be removed and replaced with tests?  The presubmit check that I'm
> > recommending is a read-only check, so I don't understand in what way it
might
> > damage a commit.
> 
> I think that presubmit checks verify the submitted change itself (like style,
> attributes, permissions, etc...). They cannot check the build result and
should
> not pretend they can. The functionality should be checked by tests. On the
other
> hand, tests cannot check code style, so presubmit hooks are handy.
> 
> I've added presubmit hook to check header style.

Sorry, I think we're talking past each other.  My suggestion is that you add a
presubmit check that verifies that (a) the contents of the enum match the
contents of histograms.xml, and that (b) there aren't any unexpected changes to
the enum, i.e. that the enum is correctly treated as append-only.

You mentioned that a presubmit check cannot verify the build result.  I'm not
suggesting that it do so.  I'm not sure what lines you're thinking along for
this -- what build result needs to be verified?  Each entry in about:flags is
already verified, by the compiler, to have an enum member associated with it --
right?  What else about the build result needs to be verified?

https://codereview.chromium.org/344883002/diff/240001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/344883002/diff/240001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2629: uma_histogram_hame,
base::HistogramBase::kUmaTargetedHistogramFlag));
You're only adding a single histogram in this CL, right?  Please continue to use
the macro to emit to the histogram, but just don't pass the name in as a
parameter, i.e. use a literal instead.

https://codereview.chromium.org/344883002/diff/240001/chrome/browser/about_fl...
File chrome/browser/about_flags_switches_histogram_ids.h (right):

https://codereview.chromium.org/344883002/diff/240001/chrome/browser/about_fl...
chrome/browser/about_flags_switches_histogram_ids.h:25: NOTE: multi-line
comments / * ... * / are not supported by the PRECOMMIT hook.
nit: I'd omit this note, fwiw; let the presubmit speak for itself, if need be.

https://codereview.chromium.org/344883002/diff/240001/tools/about_flags_switc...
File
tools/about_flags_switches_histogram_ids_checker/about_flags_switches_histogram_ids_checker.py
(right):

https://codereview.chromium.org/344883002/diff/240001/tools/about_flags_switc...
tools/about_flags_switches_histogram_ids_checker/about_flags_switches_histogram_ids_checker.py:164:
return self.results
Can you reuse some of the existing code, rather than just forking it?

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags_unittest.cc File chrome/browser/about_flags_unittest.cc (right): https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_flags_unittest.cc#newcode862 chrome/browser/about_flags_unittest.cc:862: histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml")); On 2014/07/21 22:12:59, Ilya Sherman wrote: > On ...

6 years, 5 months ago (2014-07-22 14:25:06 UTC) #67

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/180001/chrome/browser/about_fl...
chrome/browser/about_flags_unittest.cc:862:
histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml"));
On 2014/07/21 22:12:59, Ilya Sherman wrote:
> On 2014/07/21 15:22:27, alemate wrote:
> > On 2014/07/12 00:25:10, Ilya Sherman wrote:
> > > On 2014/07/11 18:13:27, alemate wrote:
> > > > On 2014/07/09 00:10:50, Ilya Sherman wrote:
> > > > > On 2014/07/08 20:45:36, alemate wrote:
> > > > > > On 2014/07/08 04:22:11, Ilya Sherman wrote:
> > > > > > > 2) It actually verifies that compiled code matches histogram.xml.
> > > > > > > That is why I've put all checks to test.
> > > > > 
> > > > > The enum doesn't need to be compiled -- it has the same values on all
> > > > platforms,
> > > > > and is easy to grep through.
> > > > 
> > > > But enum doesn't have string representation of values. Adding switch
> > > > names as values seems more readable.
> > > 
> > > It looks like trimming UMA_HISTOGRAM_ID_ from the enum entry name gives
the
> > > switch name.
> > > 
> > > > > > 3) Unlike presubmit check, this CL cannot involve any damage to
> commits
> > > > > besides
> > > > > > files in CL. So it is less dangerous.
> > > > > 
> > > > > I don't understand this comment.
> > > > 
> > > > Presubmit checks run on each developer workstation on each commit.
> > > > If there is an error in presubmit code or environment, it can damage
> > > > the commit itself.
> > > 
> > > I still don't understand.  Is this a general argument that all presubmit
> > checks
> > > should be removed and replaced with tests?  The presubmit check that I'm
> > > recommending is a read-only check, so I don't understand in what way it
> might
> > > damage a commit.
> > 
> > I think that presubmit checks verify the submitted change itself (like
style,
> > attributes, permissions, etc...). They cannot check the build result and
> should
> > not pretend they can. The functionality should be checked by tests. On the
> other
> > hand, tests cannot check code style, so presubmit hooks are handy.
> > 
> > I've added presubmit hook to check header style.
> 
> Sorry, I think we're talking past each other.  My suggestion is that you add a
> presubmit check that verifies that (a) the contents of the enum match the
> contents of histograms.xml, and that (b) there aren't any unexpected changes
to
> the enum, i.e. that the enum is correctly treated as append-only.
> 
> You mentioned that a presubmit check cannot verify the build result.  I'm not
> suggesting that it do so.  I'm not sure what lines you're thinking along for
> this -- what build result needs to be verified?  Each entry in about:flags is
> already verified, by the compiler, to have an enum member associated with it
--
> right?  What else about the build result needs to be verified?

As far as I understand, you do not like the idea of reading histograms.xml in a
unit test. And you'd like to drop this unit test completely and replace it with
precommit script, am I right?

The enum values in about_flags.cc are set by people and we need to check that
the mapping between strings (switch names, which are not constants names, but
actual strings that are passed to chrome binary) matches strings that we will
get in UMA.
This can be checked by compiled code only, because parsing about_flags.cc
requires compiler.

The precommit script you're pointing at, strict_enum_value_checker.py, is used
to gather usage of enum. But what we need to monitor is not enum, but actual
switch "strings" (I don't know what it should be called as you think that it's
not a name.).

And enum here is purely internal data. It means that someone, who adds a switch
to about_flags, is not interested in correct enum usage. If he makes a mistake
in enum value in about_flags.cc , nothing bad would happen to his functionality.
So we need to check that strings in about_flags exactly match strings in
histograms.xml. And string values are available at compile time only, so there
is no way to check it with precommit script.

And looking at precommit script that I've added here, it looks to me that manual
check of about_flags_switches_histogram_ids.h is less time-consuming then
support of 160 lines of non-trivial python code which definitely requires a test
for itself. At least for me as I'll be the owner of
about_flags_switches_histogram_ids.h ;-) (Probably I'll add it later, when I get
tired of pointing at the same errors, but I need some commit statistics.)

https://codereview.chromium.org/344883002/diff/240001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/344883002/diff/240001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2629: uma_histogram_hame,
base::HistogramBase::kUmaTargetedHistogramFlag));
On 2014/07/21 22:12:59, Ilya Sherman wrote:
> You're only adding a single histogram in this CL, right?  Please continue to
use
> the macro to emit to the histogram, but just don't pass the name in as a
> parameter, i.e. use a literal instead.

It would make this function ChromeOS-only, wouldn't it?
As far as I understood thestig@, whose idea was to move ReportCustomFlags from
chromeos:: to about_flags::, it should be less OS-specific.

Maybe you could once introduce non-caching macros? I see that caching is not
used in UMA_HISTOGRAM_SPARSE_SLOWLY() macro, so, why don't we have these flags?

https://codereview.chromium.org/344883002/diff/240001/chrome/browser/about_fl...
File chrome/browser/about_flags_switches_histogram_ids.h (right):

https://codereview.chromium.org/344883002/diff/240001/chrome/browser/about_fl...
chrome/browser/about_flags_switches_histogram_ids.h:25: NOTE: multi-line
comments / * ... * / are not supported by the PRECOMMIT hook.
On 2014/07/21 22:12:59, Ilya Sherman wrote:
> nit: I'd omit this note, fwiw; let the presubmit speak for itself, if need be.

It's rather painful to convert presubmit warnings to "multi-line comments / *
... * / are not supported". I hope this comment would save several minutes to
someone. ;-)

https://codereview.chromium.org/344883002/diff/240001/tools/about_flags_switc...
File
tools/about_flags_switches_histogram_ids_checker/about_flags_switches_histogram_ids_checker.py
(right):

https://codereview.chromium.org/344883002/diff/240001/tools/about_flags_switc...
tools/about_flags_switches_histogram_ids_checker/about_flags_switches_histogram_ids_checker.py:164:
return self.results
On 2014/07/21 22:12:59, Ilya Sherman wrote:
> Can you reuse some of the existing code, rather than just forking it?

Hmm. That means splitting strict_enum_value_checker.py (and *_test.py) into an
environment library and actual checkers. I'll look into it, but doesn't it seem
too much for all this?

Ilya Sherman

I'll try to distill our conversation from earlier today: After taking a step back to ...

6 years, 5 months ago (2014-07-22 20:37:05 UTC) #68

I'll try to distill our conversation from earlier today:

After taking a step back to think about this design more, my preference would to
be to leverage the sparse nature of the histogram and to emit 32-bit hashes of
the switches.  That allows for a much simpler (and less invasive)
implementation.  The cost is that we have to maintain a hash-to-switch-name
mapping in histograms.xml, which presumably requires some code to generate the
mapping.

However, based on our conversation, your preference is still to use a dense
mapping, and to keep the unit test.  In that case, I'll ask that you
  (1) Clean up the test code, including:
      (a) Separate out parsing the XML contents from the meat of the test.
      (b) Break the test up into multiple tests, e.g. one to verify that enum
values are not reused, one to verify that the enum labels in histograms.xml are
correct, etc.
  (2) Verify whether there are additional changes needed to use histograms.xml
as a test input data file.

Thanks.

https://chromiumcodereview.appspot.com/344883002/diff/180001/chrome/browser/a...
File chrome/browser/about_flags_unittest.cc (right):

https://chromiumcodereview.appspot.com/344883002/diff/180001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:17: #include
"third_party/libxml/chromium/libxml_utils.h"
FWIW, the Autofill code uses #include
"third_party/libjingle/source/talk/xmllite/xmlparser.h" for XML parsing.  I'm
not sure which is simpler to use.

https://chromiumcodereview.appspot.com/344883002/diff/240001/chrome/browser/a...
File chrome/browser/about_flags.cc (right):

https://chromiumcodereview.appspot.com/344883002/diff/240001/chrome/browser/a...
chrome/browser/about_flags.cc:2629: uma_histogram_hame,
base::HistogramBase::kUmaTargetedHistogramFlag));
On 2014/07/22 14:25:05, alemate wrote:
> On 2014/07/21 22:12:59, Ilya Sherman wrote:
> > You're only adding a single histogram in this CL, right?  Please continue to
> use
> > the macro to emit to the histogram, but just don't pass the name in as a
> > parameter, i.e. use a literal instead.
> 
> It would make this function ChromeOS-only, wouldn't it?
> As far as I understood thestig@, whose idea was to move ReportCustomFlags from
> chromeos:: to about_flags::, it should be less OS-specific.
> 
> Maybe you could once introduce non-caching macros? I see that caching is not
> used in UMA_HISTOGRAM_SPARSE_SLOWLY() macro, so, why don't we have these
flags?

I apologize, I forgot that the sparse histogram macro does not cache the
histogram.  Given that, it's fine to go back to using the macro, and keep the
histogram name as a parameter to this method.

sky

I like the less invasive approach. Why aren't we going with it? On Tue, Jul ...

6 years, 5 months ago (2014-07-22 21:22:44 UTC) #69

I like the less invasive approach. Why aren't we going with it?

On Tue, Jul 22, 2014 at 1:37 PM,  <isherman@chromium.org> wrote:
> I'll try to distill our conversation from earlier today:
>
> After taking a step back to think about this design more, my preference
> would to
> be to leverage the sparse nature of the histogram and to emit 32-bit hashes
> of
> the switches.  That allows for a much simpler (and less invasive)
> implementation.  The cost is that we have to maintain a hash-to-switch-name
> mapping in histograms.xml, which presumably requires some code to generate
> the
> mapping.
>
> However, based on our conversation, your preference is still to use a dense
> mapping, and to keep the unit test.  In that case, I'll ask that you
>   (1) Clean up the test code, including:
>       (a) Separate out parsing the XML contents from the meat of the test.
>       (b) Break the test up into multiple tests, e.g. one to verify that
> enum
> values are not reused, one to verify that the enum labels in histograms.xml
> are
> correct, etc.
>   (2) Verify whether there are additional changes needed to use
> histograms.xml
> as a test input data file.
>
> Thanks.
>
>
>
https://chromiumcodereview.appspot.com/344883002/diff/180001/chrome/browser/a...
> File chrome/browser/about_flags_unittest.cc (right):
>
>
https://chromiumcodereview.appspot.com/344883002/diff/180001/chrome/browser/a...
> chrome/browser/about_flags_unittest.cc:17: #include
> "third_party/libxml/chromium/libxml_utils.h"
> FWIW, the Autofill code uses #include
> "third_party/libjingle/source/talk/xmllite/xmlparser.h" for XML parsing.
>  I'm not sure which is simpler to use.
>
>
https://chromiumcodereview.appspot.com/344883002/diff/240001/chrome/browser/a...
> File chrome/browser/about_flags.cc (right):
>
>
https://chromiumcodereview.appspot.com/344883002/diff/240001/chrome/browser/a...
>
> chrome/browser/about_flags.cc:2629: uma_histogram_hame,
> base::HistogramBase::kUmaTargetedHistogramFlag));
> On 2014/07/22 14:25:05, alemate wrote:
>>
>> On 2014/07/21 22:12:59, Ilya Sherman wrote:
>> > You're only adding a single histogram in this CL, right?  Please
>
> continue to
>>
>> use
>> > the macro to emit to the histogram, but just don't pass the name in
>
> as a
>>
>> > parameter, i.e. use a literal instead.
>
>
>> It would make this function ChromeOS-only, wouldn't it?
>> As far as I understood thestig@, whose idea was to move
>
> ReportCustomFlags from
>>
>> chromeos:: to about_flags::, it should be less OS-specific.
>
>
>> Maybe you could once introduce non-caching macros? I see that caching
>
> is not
>>
>> used in UMA_HISTOGRAM_SPARSE_SLOWLY() macro, so, why don't we have
>
> these flags?
>
> I apologize, I forgot that the sparse histogram macro does not cache the
> histogram.  Given that, it's fine to go back to using the macro, and
> keep the histogram name as a parameter to this method.
>
> https://chromiumcodereview.appspot.com/344883002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Alexander Alekseev

+maruel for chrome/unit_tests.isolate On 2014/07/22 20:37:05, Ilya Sherman wrote: > I'll try to distill our ...

6 years, 4 months ago (2014-07-28 22:30:06 UTC) #70

+maruel for chrome/unit_tests.isolate

On 2014/07/22 20:37:05, Ilya Sherman wrote:
> I'll try to distill our conversation from earlier today:
> 
> After taking a step back to think about this design more, my preference would
to
> be to leverage the sparse nature of the histogram and to emit 32-bit hashes of
> the switches.  That allows for a much simpler (and less invasive)
> implementation.  The cost is that we have to maintain a hash-to-switch-name
> mapping in histograms.xml, which presumably requires some code to generate the
> mapping.
> 
> However, based on our conversation, your preference is still to use a dense
> mapping, and to keep the unit test.  In that case, I'll ask that you
>   (1) Clean up the test code, including:
>       (a) Separate out parsing the XML contents from the meat of the test.
>       (b) Break the test up into multiple tests, e.g. one to verify that enum
> values are not reused, one to verify that the enum labels in histograms.xml
are
> correct, etc.
>   (2) Verify whether there are additional changes needed to use histograms.xml
> as a test input data file.
> 
> Thanks.

Thank you for this list!

Please review.

https://codereview.chromium.org/344883002/diff/240001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/344883002/diff/240001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2629: uma_histogram_hame,
base::HistogramBase::kUmaTargetedHistogramFlag));
On 2014/07/22 20:37:05, Ilya Sherman wrote:
> On 2014/07/22 14:25:05, alemate wrote:
> > On 2014/07/21 22:12:59, Ilya Sherman wrote:
> > > You're only adding a single histogram in this CL, right?  Please continue
to
> > use
> > > the macro to emit to the histogram, but just don't pass the name in as a
> > > parameter, i.e. use a literal instead.
> > 
> > It would make this function ChromeOS-only, wouldn't it?
> > As far as I understood thestig@, whose idea was to move ReportCustomFlags
from
> > chromeos:: to about_flags::, it should be less OS-specific.
> > 
> > Maybe you could once introduce non-caching macros? I see that caching is not
> > used in UMA_HISTOGRAM_SPARSE_SLOWLY() macro, so, why don't we have these
> flags?
> 
> I apologize, I forgot that the sparse histogram macro does not cache the
> histogram.  Given that, it's fine to go back to using the macro, and keep the
> histogram name as a parameter to this method.

Done.

Ilya Sherman

Did you see Scott's question (i.e. the comment immediately preceding yours)? I'll take a look ...

6 years, 4 months ago (2014-07-28 22:47:31 UTC) #71

Alexander Alekseev

On 2014/07/22 21:22:44, sky wrote: > I like the less invasive approach. Why aren't we ...

6 years, 4 months ago (2014-07-28 23:41:38 UTC) #72

On 2014/07/22 21:22:44, sky wrote:
> I like the less invasive approach. Why aren't we going with it?

Sorry for the delayed answer.

My point is that: to build histogram you need stable dense list of buckets.
Otherwise it is problematic to compare new results with old, to understand
what was added or removed between releases, etc.

We can keep it either in chromium code or on server side. I think it is
simpler to store it in chromium source, because all dependencies would be in
a single repository.

That is why I think it is important that we would have enum
SwitchesUmaHistogramId
in chromium code. This way adding or removing command-line switches would
result in visible updates to UMA data. I also have a slender hope that
the authors of new flags would think of its cost to ChromeOS user experience.

There are several other differences in Ilya / me thoughts on the design of this
feature (Ilya, please correct me if I am wrong):

1) Ilya didn't like the idea of reading histograms.xml in unit_test.
Because this would be the first unit-test to parse histograms.xml, we do not
have
test code ready to parse it. And my CL #13 is not clear enough.
On the other hand, histograms.xml is usually analyzed by precommit hooks, so we
could add another checker there.

From my point of view, there is a problem with about_flags.cc : there is no way
to get the full lust of command-line switches used in about_flags.cc without
compiling it. After chrome restart, we do not see flags. We need real
command-line
switches. Switches are duplicated there. Some have values. And moreover,
parts of about_flags.cc are covered by #ifdefs, so it makes the analysis even
more
difficult.

So my decision was to write unit test, which would test the real compiled binary
and check that it exactly matches histograms.xml. And I still think that it is
the
most stable was to check the build result.

So that I've updated unit test code to be more clear, and I'm ready to do
further
 simplify it if you have more ideas.

Another problem is that deletion and modification of switches cannot be fully
covered
by unit_test, because previous source is not available. This can be checked by
precommit script. I've created it (see CL #13), but it is rather complicated, so
it
needs its own test. I think it is overkill for now, and I can verify changes to
about_flags_switches_histogram_ids.h manually. I don't think there are going to
be
many updates to it, so it seems to me more easy to maintain that enum manually
than 200 python lines (plus some test).

Probably I'll add this precommit later, if it turns out to be simplier to
maintain
precommit ;-) .

2) Ilya's idea to emit hashes of the switches would simplify adding of new
switches to about_flags.cc .

In my CL switch must be added to:
  about_flags.cc
  about_flags_switches_histogram_ids.h (add new enum entry)
  chrome/browser/about_flags_unittest.cc (add switch text to the
HistogramSwitchesOrdered[] )
  tools/metrics/histograms/histograms.xml (add new enum entry)

I think, that reporting hashes instead of indexes would drop the second line
from
this list (no more enum). But this would be at the cost of maintaining stable
histogram
bucket list outside of the chromium source. I think that keeping it here would
simplify
analysis and maintenance.

As far as I know, we do not already have support of comparing two histograms in
the UMA
interface. We would either need to add this feature (and maintain it), or build
comparison
outside of UMA. It all seems to be more difficult than to maintain enum
SwitchesUmaHistogramId.

That is why I hope that we can have "enum SwitchesUmaHistogramId" in chromium
code and
unit test to verify mapping between command-line switches in about_flags.cc and
strings
in histograms.xml . It seems to me that presubmit code is overkill here, but may
be your
experience suggests something other.

> On Tue, Jul 22, 2014 at 1:37 PM,  <mailto:isherman@chromium.org> wrote:
> > I'll try to distill our conversation from earlier today:
> >
> > After taking a step back to think about this design more, my preference
> > would to
> > be to leverage the sparse nature of the histogram and to emit 32-bit hashes
> > of
> > the switches.  That allows for a much simpler (and less invasive)
> > implementation.  The cost is that we have to maintain a hash-to-switch-name
> > mapping in histograms.xml, which presumably requires some code to generate
> > the
> > mapping.
> >
> > However, based on our conversation, your preference is still to use a dense
> > mapping, and to keep the unit test.  In that case, I'll ask that you
> >   (1) Clean up the test code, including:
> >       (a) Separate out parsing the XML contents from the meat of the test.
> >       (b) Break the test up into multiple tests, e.g. one to verify that
> > enum
> > values are not reused, one to verify that the enum labels in histograms.xml
> > are
> > correct, etc.
> >   (2) Verify whether there are additional changes needed to use
> > histograms.xml
> > as a test input data file.
> >
> > Thanks.

Ilya Sherman

On 2014/07/28 23:41:38, alemate wrote: > On 2014/07/22 21:22:44, sky wrote: > > I like ...

6 years, 4 months ago (2014-07-28 23:57:08 UTC) #73

On 2014/07/28 23:41:38, alemate wrote:
> On 2014/07/22 21:22:44, sky wrote:
> > I like the less invasive approach. Why aren't we going with it?
> 
> Sorry for the delayed answer.
> 
> My point is that: to build histogram you need stable dense list of buckets.
> Otherwise it is problematic to compare new results with old, to understand
> what was added or removed between releases, etc.

FWIW, I don't understand this concern.  I agree that we want a stable list,
which hashes provide.  Why do we also need a dense one?

> We can keep it either in chromium code or on server side. I think it is
> simpler to store it in chromium source, because all dependencies would be in
> a single repository.

Regardless of whether the list is dense or sparse, it will be stored in
histograms.xml, which lives in the Chromium repository.

> 
> That is why I think it is important that we would have enum
> SwitchesUmaHistogramId
> in chromium code. This way adding or removing command-line switches would
> result in visible updates to UMA data. I also have a slender hope that
> the authors of new flags would think of its cost to ChromeOS user experience.
> 
> There are several other differences in Ilya / me thoughts on the design of
this
> feature (Ilya, please correct me if I am wrong):
> 
> 1) Ilya didn't like the idea of reading histograms.xml in unit_test.
> Because this would be the first unit-test to parse histograms.xml, we do not
> have
> test code ready to parse it. And my CL #13 is not clear enough.
> On the other hand, histograms.xml is usually analyzed by precommit hooks, so
we
> could add another checker there.
> 
> From my point of view, there is a problem with about_flags.cc : there is no
way
> to get the full lust of command-line switches used in about_flags.cc without
> compiling it. After chrome restart, we do not see flags. We need real
> command-line
> switches. Switches are duplicated there. Some have values. And moreover,
> parts of about_flags.cc are covered by #ifdefs, so it makes the analysis even
> more
> difficult.
> 
> So my decision was to write unit test, which would test the real compiled
binary
> and check that it exactly matches histograms.xml. And I still think that it is
> the
> most stable was to check the build result.
> 
> So that I've updated unit test code to be more clear, and I'm ready to do
> further
>  simplify it if you have more ideas.
> 
> Another problem is that deletion and modification of switches cannot be fully
> covered
> by unit_test, because previous source is not available. This can be checked by
> precommit script. I've created it (see CL #13), but it is rather complicated,
so
> it
> needs its own test. I think it is overkill for now, and I can verify changes
to
> about_flags_switches_histogram_ids.h manually. I don't think there are going
to
> be
> many updates to it, so it seems to me more easy to maintain that enum manually
> than 200 python lines (plus some test).
> 
> Probably I'll add this precommit later, if it turns out to be simplier to
> maintain
> precommit ;-) .
> 
> 2) Ilya's idea to emit hashes of the switches would simplify adding of new
> switches to about_flags.cc .
> 
> In my CL switch must be added to:
>   about_flags.cc
>   about_flags_switches_histogram_ids.h (add new enum entry)
>   chrome/browser/about_flags_unittest.cc (add switch text to the
> HistogramSwitchesOrdered[] )
>   tools/metrics/histograms/histograms.xml (add new enum entry)
> 
> I think, that reporting hashes instead of indexes would drop the second line
> from
> this list (no more enum). But this would be at the cost of maintaining stable
> histogram
> bucket list outside of the chromium source. I think that keeping it here would
> simplify
> analysis and maintenance.

Firstly, if we use hashes, I see no reason to have the unit test be quite as
complex -- the concern you raised about people associating the wrong enum item
with a switch should no longer be relevant, since the code would
programmatically choose which string to hash, and it could just choose the
correct one.

Secondly, as I mentioned above, histograms.xml would provide a stable bucket
list within the Chromium repository.

> As far as I know, we do not already have support of comparing two histograms
in
> the UMA
> interface. We would either need to add this feature (and maintain it), or
build
> comparison
> outside of UMA. It all seems to be more difficult than to maintain enum
> SwitchesUmaHistogramId.

We do have some functionality along these lines.  If you were planning to write
manual scripts rather than using the dashboards, I'd recommend emailing
uma-team@ about your usecase.  I think there's a good chance that the dashboards
would provide, via existing functionality, the tools that you need.

> That is why I hope that we can have "enum SwitchesUmaHistogramId" in chromium
> code and
> unit test to verify mapping between command-line switches in about_flags.cc
and
> strings
> in histograms.xml . It seems to me that presubmit code is overkill here, but
may
> be your
> experience suggests something other.
> 
> 
> > On Tue, Jul 22, 2014 at 1:37 PM,  <mailto:isherman@chromium.org> wrote:
> > > I'll try to distill our conversation from earlier today:
> > >
> > > After taking a step back to think about this design more, my preference
> > > would to
> > > be to leverage the sparse nature of the histogram and to emit 32-bit
hashes
> > > of
> > > the switches.  That allows for a much simpler (and less invasive)
> > > implementation.  The cost is that we have to maintain a
hash-to-switch-name
> > > mapping in histograms.xml, which presumably requires some code to generate
> > > the
> > > mapping.
> > >
> > > However, based on our conversation, your preference is still to use a
dense
> > > mapping, and to keep the unit test.  In that case, I'll ask that you
> > >   (1) Clean up the test code, including:
> > >       (a) Separate out parsing the XML contents from the meat of the test.
> > >       (b) Break the test up into multiple tests, e.g. one to verify that
> > > enum
> > > values are not reused, one to verify that the enum labels in
histograms.xml
> > > are
> > > correct, etc.
> > >   (2) Verify whether there are additional changes needed to use
> > > histograms.xml
> > > as a test input data file.
> > >
> > > Thanks.

Alexander Alekseev

On 2014/07/28 22:47:31, Ilya Sherman wrote: > Did you see Scott's question (i.e. the comment ...

6 years, 4 months ago (2014-07-29 00:07:24 UTC) #74

Alexander Alekseev

On 2014/07/28 23:57:08, Ilya Sherman wrote: > On 2014/07/28 23:41:38, alemate wrote: > > On ...

6 years, 4 months ago (2014-07-29 00:25:54 UTC) #75

On 2014/07/28 23:57:08, Ilya Sherman wrote:
> On 2014/07/28 23:41:38, alemate wrote:
> > On 2014/07/22 21:22:44, sky wrote:
> > > I like the less invasive approach. Why aren't we going with it?
> > 
> > Sorry for the delayed answer.
> > 
> > My point is that: to build histogram you need stable dense list of buckets.
> > Otherwise it is problematic to compare new results with old, to understand
> > what was added or removed between releases, etc.
> 
> FWIW, I don't understand this concern.  I agree that we want a stable list,
> which hashes provide.  Why do we also need a dense one?

Consider two histograms:

1)
    X
X   X
X X X
1 3 5

2)

    X
X X X
2 5 6

To compare them we need to convert both of them to the set (1,2,3,5,6).
The question is whether it will be stored in chromium source, or
in some external system.

It seems that it is easier (for me) to maintain this list in chromium source.
Of course, it would be great if some other person would create and maintain that
external system ;-)


> > We can keep it either in chromium code or on server side. I think it is
> > simpler to store it in chromium source, because all dependencies would be in
> > a single repository.
> 
> Regardless of whether the list is dense or sparse, it will be stored in
> histograms.xml, which lives in the Chromium repository.
> 
> > 
> > That is why I think it is important that we would have enum
> > SwitchesUmaHistogramId
> > in chromium code. This way adding or removing command-line switches would
> > result in visible updates to UMA data. I also have a slender hope that
> > the authors of new flags would think of its cost to ChromeOS user
experience.
> > 
> > There are several other differences in Ilya / me thoughts on the design of
> this
> > feature (Ilya, please correct me if I am wrong):
> > 
> > 1) Ilya didn't like the idea of reading histograms.xml in unit_test.
> > Because this would be the first unit-test to parse histograms.xml, we do not
> > have
> > test code ready to parse it. And my CL #13 is not clear enough.
> > On the other hand, histograms.xml is usually analyzed by precommit hooks, so
> we
> > could add another checker there.
> > 
> > From my point of view, there is a problem with about_flags.cc : there is no
> way
> > to get the full lust of command-line switches used in about_flags.cc without
> > compiling it. After chrome restart, we do not see flags. We need real
> > command-line
> > switches. Switches are duplicated there. Some have values. And moreover,
> > parts of about_flags.cc are covered by #ifdefs, so it makes the analysis
even
> > more
> > difficult.
> > 
> > So my decision was to write unit test, which would test the real compiled
> binary
> > and check that it exactly matches histograms.xml. And I still think that it
is
> > the
> > most stable was to check the build result.
> > 
> > So that I've updated unit test code to be more clear, and I'm ready to do
> > further
> >  simplify it if you have more ideas.
> > 
> > Another problem is that deletion and modification of switches cannot be
fully
> > covered
> > by unit_test, because previous source is not available. This can be checked
by
> > precommit script. I've created it (see CL #13), but it is rather
complicated,
> so
> > it
> > needs its own test. I think it is overkill for now, and I can verify changes
> to
> > about_flags_switches_histogram_ids.h manually. I don't think there are going
> to
> > be
> > many updates to it, so it seems to me more easy to maintain that enum
manually
> > than 200 python lines (plus some test).
> > 
> > Probably I'll add this precommit later, if it turns out to be simplier to
> > maintain
> > precommit ;-) .
> > 
> > 2) Ilya's idea to emit hashes of the switches would simplify adding of new
> > switches to about_flags.cc .
> > 
> > In my CL switch must be added to:
> >   about_flags.cc
> >   about_flags_switches_histogram_ids.h (add new enum entry)
> >   chrome/browser/about_flags_unittest.cc (add switch text to the
> > HistogramSwitchesOrdered[] )
> >   tools/metrics/histograms/histograms.xml (add new enum entry)
> > 
> > I think, that reporting hashes instead of indexes would drop the second line
> > from
> > this list (no more enum). But this would be at the cost of maintaining
stable
> > histogram
> > bucket list outside of the chromium source. I think that keeping it here
would
> > simplify
> > analysis and maintenance.
> 
> Firstly, if we use hashes, I see no reason to have the unit test be quite as
> complex -- the concern you raised about people associating the wrong enum item
> with a switch should no longer be relevant, since the code would
> programmatically choose which string to hash, and it could just choose the
> correct one.

We would still need to verify that all switches from about_flags.cc are found in
histograms.xml. Due to complexity of about_flags we need to compile it to get
the list. 

We could drop the list of strings from unit test as the cost of having precommit
hook.
But dropping unit test would make mapping unstable.


> Secondly, as I mentioned above, histograms.xml would provide a stable bucket
> list within the Chromium repository.

Yes, if we would have precommit hook.


> > As far as I know, we do not already have support of comparing two histograms
> in
> > the UMA
> > interface. We would either need to add this feature (and maintain it), or
> build
> > comparison
> > outside of UMA. It all seems to be more difficult than to maintain enum
> > SwitchesUmaHistogramId.
> 
> We do have some functionality along these lines.  If you were planning to
write
> manual scripts rather than using the dashboards, I'd recommend emailing
> uma-team@ about your usecase.  I think there's a good chance that the
dashboards
> would provide, via existing functionality, the tools that you need.

As far as I know, they strongly suggest to build results on the chromium side
as close to the needed report format as possible. Because it reduces load
at our side.

> > That is why I hope that we can have "enum SwitchesUmaHistogramId" in
chromium
> > code and
> > unit test to verify mapping between command-line switches in about_flags.cc
> and
> > strings
> > in histograms.xml . It seems to me that presubmit code is overkill here, but
> may
> > be your
> > experience suggests something other.
> > 
> > 
> > > On Tue, Jul 22, 2014 at 1:37 PM,  <mailto:isherman@chromium.org> wrote:
> > > > I'll try to distill our conversation from earlier today:
> > > >
> > > > After taking a step back to think about this design more, my preference
> > > > would to
> > > > be to leverage the sparse nature of the histogram and to emit 32-bit
> hashes
> > > > of
> > > > the switches.  That allows for a much simpler (and less invasive)
> > > > implementation.  The cost is that we have to maintain a
> hash-to-switch-name
> > > > mapping in histograms.xml, which presumably requires some code to
generate
> > > > the
> > > > mapping.
> > > >
> > > > However, based on our conversation, your preference is still to use a
> dense
> > > > mapping, and to keep the unit test.  In that case, I'll ask that you
> > > >   (1) Clean up the test code, including:
> > > >       (a) Separate out parsing the XML contents from the meat of the
test.
> > > >       (b) Break the test up into multiple tests, e.g. one to verify that
> > > > enum
> > > > values are not reused, one to verify that the enum labels in
> histograms.xml
> > > > are
> > > > correct, etc.
> > > >   (2) Verify whether there are additional changes needed to use
> > > > histograms.xml
> > > > as a test input data file.
> > > >
> > > > Thanks.

Ilya Sherman

On 2014/07/29 00:25:54, alemate wrote: > On 2014/07/28 23:57:08, Ilya Sherman wrote: > > On ...

6 years, 4 months ago (2014-07-29 00:50:06 UTC) #76

On 2014/07/29 00:25:54, alemate wrote:
> On 2014/07/28 23:57:08, Ilya Sherman wrote:
> > On 2014/07/28 23:41:38, alemate wrote:
> > > On 2014/07/22 21:22:44, sky wrote:
> > > > I like the less invasive approach. Why aren't we going with it?
> > > 
> > > Sorry for the delayed answer.
> > > 
> > > My point is that: to build histogram you need stable dense list of
buckets.
> > > Otherwise it is problematic to compare new results with old, to understand
> > > what was added or removed between releases, etc.
> > 
> > FWIW, I don't understand this concern.  I agree that we want a stable list,
> > which hashes provide.  Why do we also need a dense one?
> 
> Consider two histograms:
> 
> 1)
>     X
> X   X
> X X X
> 1 3 5
> 
> 2)
> 
>     X
> X X X
> 2 5 6
> 
> To compare them we need to convert both of them to the set (1,2,3,5,6).

I still don't understand what this means.  The two histograms are equally
accurately represented as below, and in this representation, they are clearly
trivially easy to merge or compare.  The human-readable labels assigned to
buckets have nothing to do with the algorithms for comparing the histograms
themselves.

1)
        X
X       X
X   X   X
1 2 3 4 5 6

2)

          X
  X     X X
1 2 3 4 5 6

> The question is whether it will be stored in chromium source, or
> in some external system.
> 
> It seems that it is easier (for me) to maintain this list in chromium source.
> Of course, it would be great if some other person would create and maintain
that
> external system ;-)
> 
> 
> > > We can keep it either in chromium code or on server side. I think it is
> > > simpler to store it in chromium source, because all dependencies would be
in
> > > a single repository.
> > 
> > Regardless of whether the list is dense or sparse, it will be stored in
> > histograms.xml, which lives in the Chromium repository.
> > 
> > > 
> > > That is why I think it is important that we would have enum
> > > SwitchesUmaHistogramId
> > > in chromium code. This way adding or removing command-line switches would
> > > result in visible updates to UMA data. I also have a slender hope that
> > > the authors of new flags would think of its cost to ChromeOS user
> experience.
> > > 
> > > There are several other differences in Ilya / me thoughts on the design of
> > this
> > > feature (Ilya, please correct me if I am wrong):
> > > 
> > > 1) Ilya didn't like the idea of reading histograms.xml in unit_test.
> > > Because this would be the first unit-test to parse histograms.xml, we do
not
> > > have
> > > test code ready to parse it. And my CL #13 is not clear enough.
> > > On the other hand, histograms.xml is usually analyzed by precommit hooks,
so
> > we
> > > could add another checker there.
> > > 
> > > From my point of view, there is a problem with about_flags.cc : there is
no
> > way
> > > to get the full lust of command-line switches used in about_flags.cc
without
> > > compiling it. After chrome restart, we do not see flags. We need real
> > > command-line
> > > switches. Switches are duplicated there. Some have values. And moreover,
> > > parts of about_flags.cc are covered by #ifdefs, so it makes the analysis
> even
> > > more
> > > difficult.
> > > 
> > > So my decision was to write unit test, which would test the real compiled
> > binary
> > > and check that it exactly matches histograms.xml. And I still think that
it
> is
> > > the
> > > most stable was to check the build result.
> > > 
> > > So that I've updated unit test code to be more clear, and I'm ready to do
> > > further
> > >  simplify it if you have more ideas.
> > > 
> > > Another problem is that deletion and modification of switches cannot be
> fully
> > > covered
> > > by unit_test, because previous source is not available. This can be
checked
> by
> > > precommit script. I've created it (see CL #13), but it is rather
> complicated,
> > so
> > > it
> > > needs its own test. I think it is overkill for now, and I can verify
changes
> > to
> > > about_flags_switches_histogram_ids.h manually. I don't think there are
going
> > to
> > > be
> > > many updates to it, so it seems to me more easy to maintain that enum
> manually
> > > than 200 python lines (plus some test).
> > > 
> > > Probably I'll add this precommit later, if it turns out to be simplier to
> > > maintain
> > > precommit ;-) .
> > > 
> > > 2) Ilya's idea to emit hashes of the switches would simplify adding of new
> > > switches to about_flags.cc .
> > > 
> > > In my CL switch must be added to:
> > >   about_flags.cc
> > >   about_flags_switches_histogram_ids.h (add new enum entry)
> > >   chrome/browser/about_flags_unittest.cc (add switch text to the
> > > HistogramSwitchesOrdered[] )
> > >   tools/metrics/histograms/histograms.xml (add new enum entry)
> > > 
> > > I think, that reporting hashes instead of indexes would drop the second
line
> > > from
> > > this list (no more enum). But this would be at the cost of maintaining
> stable
> > > histogram
> > > bucket list outside of the chromium source. I think that keeping it here
> would
> > > simplify
> > > analysis and maintenance.
> > 
> > Firstly, if we use hashes, I see no reason to have the unit test be quite as
> > complex -- the concern you raised about people associating the wrong enum
item
> > with a switch should no longer be relevant, since the code would
> > programmatically choose which string to hash, and it could just choose the
> > correct one.
> 
> We would still need to verify that all switches from about_flags.cc are found
in
> histograms.xml. Due to complexity of about_flags we need to compile it to get
> the list. 
> 
> We could drop the list of strings from unit test as the cost of having
precommit
> hook.
> But dropping unit test would make mapping unstable.

Perhaps you mean something different by "unstable".  I take "stable" to mean
that the flag "--enable-feature-foo" will always map to hash 0xDEADBEEF.  That
has nothing to do with mapping 0xDEADBEEF back to "--enable-feature-foo" in
histograms.xml.

As far as updating histograms.xml goes, I agree that we'd want some automated
way to do so.  However, I'm not convinced that a unit test would be the most
appropriate option if we're hashing strings.  Even if we do leave it as a unit
test, I'd lean toward not hardcoding the list of flags, since the test would
actually be more robust if it would just automatically check every flag
available on the platform it's being run on.

> > Secondly, as I mentioned above, histograms.xml would provide a stable bucket
> > list within the Chromium repository.
> 
> Yes, if we would have precommit hook.
> 
> 
> > > As far as I know, we do not already have support of comparing two
histograms
> > in
> > > the UMA
> > > interface. We would either need to add this feature (and maintain it), or
> > build
> > > comparison
> > > outside of UMA. It all seems to be more difficult than to maintain enum
> > > SwitchesUmaHistogramId.
> > 
> > We do have some functionality along these lines.  If you were planning to
> write
> > manual scripts rather than using the dashboards, I'd recommend emailing
> > uma-team@ about your usecase.  I think there's a good chance that the
> dashboards
> > would provide, via existing functionality, the tools that you need.
> 
> As far as I know, they strongly suggest to build results on the chromium side
> as close to the needed report format as possible. Because it reduces load
> at our side.

Why does your report format need to have dense buckets?  Wouldn't you prefer the
report format to have human-readable strings, and therefore be agnostic to the
invisible machine representation that's used in the backend?  This seems like a
conversation that might be better to continue on the uma-team@ mailing list.

> > > That is why I hope that we can have "enum SwitchesUmaHistogramId" in
> chromium
> > > code and
> > > unit test to verify mapping between command-line switches in
about_flags.cc
> > and
> > > strings
> > > in histograms.xml . It seems to me that presubmit code is overkill here,
but
> > may
> > > be your
> > > experience suggests something other.
> > > 
> > > 
> > > > On Tue, Jul 22, 2014 at 1:37 PM,  <mailto:isherman@chromium.org> wrote:
> > > > > I'll try to distill our conversation from earlier today:
> > > > >
> > > > > After taking a step back to think about this design more, my
preference
> > > > > would to
> > > > > be to leverage the sparse nature of the histogram and to emit 32-bit
> > hashes
> > > > > of
> > > > > the switches.  That allows for a much simpler (and less invasive)
> > > > > implementation.  The cost is that we have to maintain a
> > hash-to-switch-name
> > > > > mapping in histograms.xml, which presumably requires some code to
> generate
> > > > > the
> > > > > mapping.
> > > > >
> > > > > However, based on our conversation, your preference is still to use a
> > dense
> > > > > mapping, and to keep the unit test.  In that case, I'll ask that you
> > > > >   (1) Clean up the test code, including:
> > > > >       (a) Separate out parsing the XML contents from the meat of the
> test.
> > > > >       (b) Break the test up into multiple tests, e.g. one to verify
that
> > > > > enum
> > > > > values are not reused, one to verify that the enum labels in
> > histograms.xml
> > > > > are
> > > > > correct, etc.
> > > > >   (2) Verify whether there are additional changes needed to use
> > > > > histograms.xml
> > > > > as a test input data file.
> > > > >
> > > > > Thanks.

Ilya Sherman

W.r.t. the code if you keep the current approach: https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/about_flags_unittest.cc File chrome/browser/about_flags_unittest.cc (right): https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/about_flags_unittest.cc#newcode402 chrome/browser/about_flags_unittest.cc:402: ...

6 years, 4 months ago (2014-07-29 07:16:46 UTC) #77

M-A Ruel

.isolate lgtm https://codereview.chromium.org/344883002/diff/300001/chrome/unit_tests.isolate File chrome/unit_tests.isolate (right): https://codereview.chromium.org/344883002/diff/300001/chrome/unit_tests.isolate#newcode17 chrome/unit_tests.isolate:17: '../tools/metrics/histograms/histograms.xml', It's needed in android too?

6 years, 4 months ago (2014-07-29 19:19:41 UTC) #78

Alexander Alekseev

On 2014/07/29 00:50:06, Ilya Sherman wrote: > On 2014/07/29 00:25:54, alemate wrote: > > On ...

6 years, 4 months ago (2014-07-31 00:17:53 UTC) #79

On 2014/07/29 00:50:06, Ilya Sherman wrote:
> On 2014/07/29 00:25:54, alemate wrote:
> > On 2014/07/28 23:57:08, Ilya Sherman wrote:
> > > On 2014/07/28 23:41:38, alemate wrote:
> > > > On 2014/07/22 21:22:44, sky wrote:
> > > > > I like the less invasive approach. Why aren't we going with it?
> > > > 
> > > > Sorry for the delayed answer.
> > > > 
> > > > My point is that: to build histogram you need stable dense list of
> buckets.
> > > > Otherwise it is problematic to compare new results with old, to
understand
> > > > what was added or removed between releases, etc.
> > > 
> > > FWIW, I don't understand this concern.  I agree that we want a stable
list,
> > > which hashes provide.  Why do we also need a dense one?
> > 
> > Consider two histograms:
> > 
> > 1)
> >     X
> > X   X
> > X X X
> > 1 3 5
> > 
> > 2)
> > 
> >     X
> > X X X
> > 2 5 6
> > 
> > To compare them we need to convert both of them to the set (1,2,3,5,6).
> 
> I still don't understand what this means.  The two histograms are equally
> accurately represented as below, and in this representation, they are clearly
> trivially easy to merge or compare.  The human-readable labels assigned to
> buckets have nothing to do with the algorithms for comparing the histograms
> themselves.
> 
> 1)
>         X
> X       X
> X   X   X
> 1 2 3 4 5 6
> 
> 2)
> 
>           X
>   X     X X
> 1 2 3 4 5 6
> 
> > The question is whether it will be stored in chromium source, or
> > in some external system.
> > 
> > It seems that it is easier (for me) to maintain this list in chromium
source.
> > Of course, it would be great if some other person would create and maintain
> that
> > external system ;-)
> > 
> > 
> > > > We can keep it either in chromium code or on server side. I think it is
> > > > simpler to store it in chromium source, because all dependencies would
be
> in
> > > > a single repository.
> > > 
> > > Regardless of whether the list is dense or sparse, it will be stored in
> > > histograms.xml, which lives in the Chromium repository.
> > > 
> > > > 
> > > > That is why I think it is important that we would have enum
> > > > SwitchesUmaHistogramId
> > > > in chromium code. This way adding or removing command-line switches
would
> > > > result in visible updates to UMA data. I also have a slender hope that
> > > > the authors of new flags would think of its cost to ChromeOS user
> > experience.
> > > > 
> > > > There are several other differences in Ilya / me thoughts on the design
of
> > > this
> > > > feature (Ilya, please correct me if I am wrong):
> > > > 
> > > > 1) Ilya didn't like the idea of reading histograms.xml in unit_test.
> > > > Because this would be the first unit-test to parse histograms.xml, we do
> not
> > > > have
> > > > test code ready to parse it. And my CL #13 is not clear enough.
> > > > On the other hand, histograms.xml is usually analyzed by precommit
hooks,
> so
> > > we
> > > > could add another checker there.
> > > > 
> > > > From my point of view, there is a problem with about_flags.cc : there is
> no
> > > way
> > > > to get the full lust of command-line switches used in about_flags.cc
> without
> > > > compiling it. After chrome restart, we do not see flags. We need real
> > > > command-line
> > > > switches. Switches are duplicated there. Some have values. And moreover,
> > > > parts of about_flags.cc are covered by #ifdefs, so it makes the analysis
> > even
> > > > more
> > > > difficult.
> > > > 
> > > > So my decision was to write unit test, which would test the real
compiled
> > > binary
> > > > and check that it exactly matches histograms.xml. And I still think that
> it
> > is
> > > > the
> > > > most stable was to check the build result.
> > > > 
> > > > So that I've updated unit test code to be more clear, and I'm ready to
do
> > > > further
> > > >  simplify it if you have more ideas.
> > > > 
> > > > Another problem is that deletion and modification of switches cannot be
> > fully
> > > > covered
> > > > by unit_test, because previous source is not available. This can be
> checked
> > by
> > > > precommit script. I've created it (see CL #13), but it is rather
> > complicated,
> > > so
> > > > it
> > > > needs its own test. I think it is overkill for now, and I can verify
> changes
> > > to
> > > > about_flags_switches_histogram_ids.h manually. I don't think there are
> going
> > > to
> > > > be
> > > > many updates to it, so it seems to me more easy to maintain that enum
> > manually
> > > > than 200 python lines (plus some test).
> > > > 
> > > > Probably I'll add this precommit later, if it turns out to be simplier
to
> > > > maintain
> > > > precommit ;-) .
> > > > 
> > > > 2) Ilya's idea to emit hashes of the switches would simplify adding of
new
> > > > switches to about_flags.cc .
> > > > 
> > > > In my CL switch must be added to:
> > > >   about_flags.cc
> > > >   about_flags_switches_histogram_ids.h (add new enum entry)
> > > >   chrome/browser/about_flags_unittest.cc (add switch text to the
> > > > HistogramSwitchesOrdered[] )
> > > >   tools/metrics/histograms/histograms.xml (add new enum entry)
> > > > 
> > > > I think, that reporting hashes instead of indexes would drop the second
> line
> > > > from
> > > > this list (no more enum). But this would be at the cost of maintaining
> > stable
> > > > histogram
> > > > bucket list outside of the chromium source. I think that keeping it here
> > would
> > > > simplify
> > > > analysis and maintenance.
> > > 
> > > Firstly, if we use hashes, I see no reason to have the unit test be quite
as
> > > complex -- the concern you raised about people associating the wrong enum
> item
> > > with a switch should no longer be relevant, since the code would
> > > programmatically choose which string to hash, and it could just choose the
> > > correct one.
> > 
> > We would still need to verify that all switches from about_flags.cc are
found
> in
> > histograms.xml. Due to complexity of about_flags we need to compile it to
get
> > the list. 
> > 
> > We could drop the list of strings from unit test as the cost of having
> precommit
> > hook.
> > But dropping unit test would make mapping unstable.
> 
> Perhaps you mean something different by "unstable".  I take "stable" to mean
> that the flag "--enable-feature-foo" will always map to hash 0xDEADBEEF.  That
> has nothing to do with mapping 0xDEADBEEF back to "--enable-feature-foo" in
> histograms.xml.

For me the expected result here is histogram. And "stability" is to be able to
store one histogram image, then another one a year later, put them on the same
page and be able to visually compare them.

That is why I am trying to build the final result here and never depend on
external
teams.

> As far as updating histograms.xml goes, I agree that we'd want some automated
> way to do so.  However, I'm not convinced that a unit test would be the most
> appropriate option if we're hashing strings.  Even if we do leave it as a unit
> test, I'd lean toward not hardcoding the list of flags, since the test would
> actually be more robust if it would just automatically check every flag
> available on the platform it's being run on.

The fact that list of strings in about_flags.cc is needed (or is not needed)
doesn't depend on the type of bucket ID we use. I've added it just to be sure
that the person, who would modify histograms.xml, would have to modify some
exemplary list (which doesn't depend on #ifdef's) . Just to be sure he would
read explanations around.

We can drop that list right now, but I would better leave it just to have one
more check here.


> > > Secondly, as I mentioned above, histograms.xml would provide a stable
bucket
> > > list within the Chromium repository.
> > 
> > Yes, if we would have precommit hook.
> > 
> > 
> > > > As far as I know, we do not already have support of comparing two
> histograms
> > > in
> > > > the UMA
> > > > interface. We would either need to add this feature (and maintain it),
or
> > > build
> > > > comparison
> > > > outside of UMA. It all seems to be more difficult than to maintain enum
> > > > SwitchesUmaHistogramId.
> > > 
> > > We do have some functionality along these lines.  If you were planning to
> > write
> > > manual scripts rather than using the dashboards, I'd recommend emailing
> > > uma-team@ about your usecase.  I think there's a good chance that the
> > dashboards
> > > would provide, via existing functionality, the tools that you need.
> > 
> > As far as I know, they strongly suggest to build results on the chromium
side
> > as close to the needed report format as possible. Because it reduces load
> > at our side.
> 
> Why does your report format need to have dense buckets?  Wouldn't you prefer
the
> report format to have human-readable strings, and therefore be agnostic to the
> invisible machine representation that's used in the backend?  This seems like
a
> conversation that might be better to continue on the uma-team@ mailing list.

I had conversations with them a few times, and expect current approach to be
much
faster and reliable way to get the result.

Alexander Alekseev

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/about_flags_unittest.cc File chrome/browser/about_flags_unittest.cc (right): https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/about_flags_unittest.cc#newcode402 chrome/browser/about_flags_unittest.cc:402: for (SwitchesHistogramIDs::const_iterator i = switch_histogram_id.begin(); On 2014/07/29 07:16:45, Ilya ...

6 years, 4 months ago (2014-07-31 00:18:08 UTC) #80

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
File chrome/browser/about_flags_unittest.cc (right):

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:402: for
(SwitchesHistogramIDs::const_iterator i = switch_histogram_id.begin();
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> nit: Please use "it" or "iter" for generic iterator names; "i" makes the
reader
> think of an integer variable.

Done.

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:414: } else {
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> nit: Please add ASSERT_EQ(id, histogram_id_to_switch_.size());

The order of IDs in about_flags is undefined, so it is not right.
(and some IDs might be missing)

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:439: ScopedVector<std::string>
histogram_id_to_switch_;
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> Why is this a ScopedVector, rather than just a regular vector;
> or better yet, a map?

This is scoped vector to store the special value "unused", which usually means
that this is an ID of switch under #ifdef . Or may be a  deprecated switch.

This can be a map, but it is simplier to check that value exists. (i.e. you do
not need to use lower_bound() and check iterators).

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:549: const std::string kDD("--");
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> nit: Please name this something like "kDoubleDash" rather than the cryptic
"kDD"
> plus a comment.  In general, if you find yourself needing to write a comment
to
> explain a variable name, you should probably name the variable more clearly
> instead.

Yes, you're right. Done.

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:807: EXPECT_STREQ("",
histogram_id_to_switch_[0]->c_str());
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> nit: Prefer std::string() to ""

Done.

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:809:
EXPECT_FALSE(histogram_id_to_switch_[2]); /* RESERVED2 */
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> Why is this RESERVED2 rather than RESERVED1?

The idea was to reflect index in constant name.
But I'm OK to rename it.

Done.

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:811:
EXPECT_FALSE(histogram_id_to_switch_[4]); /* RESERVED4 */
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> Please assert that the array size is at least 5, since you index into it to at
> least there.

Done.

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:818: <<
histogram_id_to_switch_[i]->c_str()
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> nit: No need for c_str().

This is because of ScopedVector .

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:836: // Returns empty map on error.
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> Please expand this comment to be more detailed.

Done.

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:837: std::map<int, std::string>
ParseEnumFromHistogramsXml(
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> Please move helper functions into the anonymous namespace.

Done.

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:843: bool success = true;
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> It looks like you can entirely replace this with early return statements,
which
> IMO would be clearer (as well as briefer).

This is to report maximum number of errors on a single run (not to stop on the
first one).

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:856: << entries_index << "): No 'value'
attribute.";
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> Did you mean to use an EXPECT_ or ASSERT_ statement?  Tests shouldn't print
> output that isn't testing assertions.

Done.

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:920: break;
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> Please document what these four control statements are doing.

Done.

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:942: .AppendASCII("histograms.xml");
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> nit: I don't think this formatting is quite right -- the continued lines
should
> be indented a total of six spaces, rather than 33.

This is really strange, but it is a result of clang-format.
Set to 6 spaces manually.

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
chrome/browser/about_flags_unittest.cc:968: for (std::map<int,
std::string>::reverse_iterator ri =
On 2014/07/29 07:16:45, Ilya Sherman wrote:
> Again, please use "it" or "iter" for the iterator name.

Done.

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/unit_test...
File chrome/unit_tests.isolate (right):

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/unit_test...
chrome/unit_tests.isolate:17: '../tools/metrics/histograms/histograms.xml',
On 2014/07/29 19:19:41, M-A Ruel wrote:
> It's needed in android too?

If AboutFlagsTest unittest is run on android is is also needed.
The idea is to check global flags consistency.

Ilya Sherman

On 2014/07/31 00:17:53, alemate wrote: > For me the expected result here is histogram. And ...

6 years, 4 months ago (2014-07-31 00:25:11 UTC) #81

Ilya Sherman

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/about_flags_unittest.cc File chrome/browser/about_flags_unittest.cc (right): https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/about_flags_unittest.cc#newcode414 chrome/browser/about_flags_unittest.cc:414: } else { On 2014/07/31 00:18:08, alemate wrote: > ...

6 years, 4 months ago (2014-07-31 00:37:52 UTC) #82

Alexander Alekseev

On 2014/07/31 00:37:52, Ilya Sherman wrote: > https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/about_flags_unittest.cc > File chrome/browser/about_flags_unittest.cc (right): > > https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/about_flags_unittest.cc#newcode414 ...

6 years, 4 months ago (2014-07-31 01:32:07 UTC) #83

On 2014/07/31 00:37:52, Ilya Sherman wrote:
>
https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
> File chrome/browser/about_flags_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
> chrome/browser/about_flags_unittest.cc:414: } else {
> On 2014/07/31 00:18:08, alemate wrote:
> > On 2014/07/29 07:16:45, Ilya Sherman wrote:
> > > nit: Please add ASSERT_EQ(id, histogram_id_to_switch_.size());
> > 
> > The order of IDs in about_flags is undefined, so it is not right.
> > (and some IDs might be missing)
> 
> In that case, it really sounds like you ought to use a map.

But the order of IDs in enum and in histograms.xml is strict. So it seems easier
to compute maps
difference between two maps when one of them is implemented
as array. When iterating you get all missing nodes for free.
And array implementation is also free here, as the map is dense.


> 
>
https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
> chrome/browser/about_flags_unittest.cc:439: ScopedVector<std::string>
> histogram_id_to_switch_;
> On 2014/07/31 00:18:08, alemate wrote:
> > On 2014/07/29 07:16:45, Ilya Sherman wrote:
> > > Why is this a ScopedVector, rather than just a regular vector;
> > > or better yet, a map?
> > 
> > This is scoped vector to store the special value "unused", which usually
means
> > that this is an ID of switch under #ifdef . Or may be a  deprecated switch.
> > 
> > This can be a map, but it is simplier to check that value exists. (i.e. you
do
> > not need to use lower_bound() and check iterators).
> 
> Checking whether a value exists in a map is easy: my_map.count(value).  That's
> also much clearer than having special/magic "unused" values.
> 
>
https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/browser/a...
> chrome/browser/about_flags_unittest.cc:818: <<
> histogram_id_to_switch_[i]->c_str()
> On 2014/07/31 00:18:08, alemate wrote:
> > On 2014/07/29 07:16:45, Ilya Sherman wrote:
> > > nit: No need for c_str().
> > 
> > This is because of ScopedVector .
> 
> I don't understand -- why does a ScopedVector require you to use c_str()? 
> What's wrong with accessing the string value in the vector directly, i.e.
> "*histogram_id_to_switch_[i]"?

Yes, you're right. Done.

https://codereview.chromium.org/344883002/diff/320001/chrome/browser/about_fl...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/320001/chrome/browser/about_fl...
chrome/browser/about_flags_unittest.cc:43: XmlReader& reader);
On 2014/07/31 00:37:51, Ilya Sherman wrote:
> Please define the function here, rather than forward-declaring it.

Done.

https://codereview.chromium.org/344883002/diff/320001/chrome/browser/about_fl...
chrome/browser/about_flags_unittest.cc:56: XmlReader& histograms_xml) {
On 2014/07/31 00:37:51, Ilya Sherman wrote:
> nit: Variables should be passed either by const-reference or by pointer;
pretty
> much never by reference.

Done.

https://codereview.chromium.org/344883002/diff/320001/chrome/browser/about_fl...
chrome/browser/about_flags_unittest.cc:59: // Implement simple DFS.
On 2014/07/31 00:37:51, Ilya Sherman wrote:
> nit: "DFS" -> "depth first search"

Done.

https://codereview.chromium.org/344883002/diff/320001/chrome/browser/about_fl...
chrome/browser/about_flags_unittest.cc:116: XmlReader& reader) {
On 2014/07/31 00:37:51, Ilya Sherman wrote:
> Ditto.

Done.

M-A Ruel

https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/unit_tests.isolate File chrome/unit_tests.isolate (right): https://chromiumcodereview.appspot.com/344883002/diff/300001/chrome/unit_tests.isolate#newcode17 chrome/unit_tests.isolate:17: '../tools/metrics/histograms/histograms.xml', On 2014/07/31 00:18:08, alemate wrote: > On 2014/07/29 ...

6 years, 4 months ago (2014-07-31 13:29:26 UTC) #84

Alexander Alekseev

I've updated CL to report hashes. Please review.

6 years, 4 months ago (2014-08-05 09:39:31 UTC) #85

sky

https://codereview.chromium.org/344883002/diff/360001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/360001/chrome/browser/about_flags.cc#newcode1522 chrome/browser/about_flags.cc:1522: UMA_HISTOGRAM_ID_kEnableAccessibilityTabSwitcher) Why are you doing this change? https://codereview.chromium.org/344883002/diff/360001/chrome/browser/about_flags.h File ...

6 years, 4 months ago (2014-08-05 16:49:15 UTC) #86

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/360001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/360001/chrome/browser/about_flags.cc#newcode1522 chrome/browser/about_flags.cc:1522: UMA_HISTOGRAM_ID_kEnableAccessibilityTabSwitcher) On 2014/08/05 16:49:14, sky wrote: > Why are ...

6 years, 4 months ago (2014-08-05 17:03:07 UTC) #87

Ilya Sherman

https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (left): https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_flags.cc#oldcode1528 chrome/browser/about_flags.cc:1528: }, This diff seems unrelated to your CL. https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_flags.cc ...

6 years, 4 months ago (2014-08-05 20:14:47 UTC) #88

Alexei Svitkine (slow)

https://codereview.chromium.org/344883002/diff/380001/tools/metrics/histograms/verify_enum_custom_flags.py File tools/metrics/histograms/verify_enum_custom_flags.py (right): https://codereview.chromium.org/344883002/diff/380001/tools/metrics/histograms/verify_enum_custom_flags.py#newcode236 tools/metrics/histograms/verify_enum_custom_flags.py:236: return self.results On 2014/08/05 20:14:47, Ilya Sherman wrote: > ...

6 years, 4 months ago (2014-08-05 20:47:52 UTC) #89

Ilya Sherman

https://codereview.chromium.org/344883002/diff/380001/tools/metrics/histograms/verify_enum_custom_flags.py File tools/metrics/histograms/verify_enum_custom_flags.py (right): https://codereview.chromium.org/344883002/diff/380001/tools/metrics/histograms/verify_enum_custom_flags.py#newcode236 tools/metrics/histograms/verify_enum_custom_flags.py:236: return self.results On 2014/08/05 20:47:52, Alexei Svitkine wrote: > ...

6 years, 4 months ago (2014-08-05 20:50:36 UTC) #90

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (left): https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_flags.cc#oldcode1528 chrome/browser/about_flags.cc:1528: }, On 2014/08/05 20:14:46, Ilya Sherman wrote: > This ...

6 years, 4 months ago (2014-08-07 23:24:56 UTC) #91

Ilya Sherman

https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (left): https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_flags.cc#oldcode1528 chrome/browser/about_flags.cc:1528: }, On 2014/08/07 23:24:55, alemate wrote: > On 2014/08/05 ...

6 years, 4 months ago (2014-08-08 03:49:46 UTC) #92

https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1528: },
On 2014/08/07 23:24:55, alemate wrote:
> On 2014/08/05 20:14:46, Ilya Sherman wrote:
> > This diff seems unrelated to your CL.
> 
> As this CL doesn't have explicit UMA IDs, this part looks irrelevant.
> But at the beginning we agreed that I would drop this (unused) part,
> and it saves extra ID from UMA.
> 
> Do you think we should reserve ID for this switch?

If this switch is obsolete, I'd prefer that you remove it in a separate, tiny
CL, since it's easy for the change to get lost in a large CL like this one.

https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2276: crc32(0,
On 2014/08/07 23:24:56, alemate wrote:
> On 2014/08/05 20:14:46, Ilya Sherman wrote:
> > Optional: md5 is more commonly used for UMA code.  It's fine to use any hash
> > function you like here, but you might want to at least double-check whether
> > Dremel natively supports crc32 before using it.
> 
> On a server-side it looks like uint32, so it doesn't need special suport, does
> it?

The only reason that special support might be useful is that it's convenient for
users to be able to type something like CRC("--enable-my-feature") in Dremel,
without having to compute the CRC themselves.

> Also, MD5 doesn't fit 32bit integer, so what ID type should I use here?

For other UMA code, we just trim the MD5 hash as needed.

https://codereview.chromium.org/344883002/diff/380001/tools/metrics/histogram...
File tools/metrics/histograms/verify_enum_custom_flags.py (right):

https://codereview.chromium.org/344883002/diff/380001/tools/metrics/histogram...
tools/metrics/histograms/verify_enum_custom_flags.py:236: return self.results
On 2014/08/07 23:24:56, alemate wrote:
> On 2014/08/05 20:14:47, Ilya Sherman wrote:
> > IMO this presubmit script is not needed now that you're using hashes, since
> > there aren't any collisions expected.  The reason that the other approach
> needed
> > a presubmit check was that one person accidentally inserting an entry in the
> > wrong spot in the enum would ruin the entire data collection process. 
That's
> > not a concern with hashes.
> 
> Unit test cannot check that values are not removed.
> So I would better add this check.

(a) None of the reviewers of this file would be likely to LGTM a CL that removed
a switch.
(b) Nobody editing this file is likely to accidentally remove a switch
(c) Even if a switch were accidentally removed, it would be easy to restore,
e.g. using code history.
(d) No data is corrupted if the histograms.xml file is not up-to-date -- it's
purely metadata.

For all of these reasons, I don't think it's worthwhile to have over 200 lines
of code (plus tests) just to verify that values are not removed.  I strongly
encourage you to either keep the C++ unittest or to write a complete presubmit
check; but not both.

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:7: #include <stdint.h>
nit: This is already included in the header file.

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2277: return base::checked_cast<unsigned>(
nit: unsigned -> uint32_t

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
File chrome/browser/about_flags_histogram_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
chrome/browser/about_flags_histogram_unittest.cc:11: #include
"base/prefs/testing_pref_service.h"*/
Please update this.

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
chrome/browser/about_flags_histogram_unittest.cc:20: //#include
"grit/chromium_strings.h"
Please update this.

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (left): https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_flags.cc#oldcode1528 chrome/browser/about_flags.cc:1528: }, On 2014/08/08 03:49:45, Ilya Sherman wrote: > On ...

6 years, 4 months ago (2014-08-09 01:30:01 UTC) #93

https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/344883002/diff/380001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1528: },
On 2014/08/08 03:49:45, Ilya Sherman wrote:
> On 2014/08/07 23:24:55, alemate wrote:
> > On 2014/08/05 20:14:46, Ilya Sherman wrote:
> > > This diff seems unrelated to your CL.
> > 
> > As this CL doesn't have explicit UMA IDs, this part looks irrelevant.
> > But at the beginning we agreed that I would drop this (unused) part,
> > and it saves extra ID from UMA.
> > 
> > Do you think we should reserve ID for this switch?
> 
> If this switch is obsolete, I'd prefer that you remove it in a separate, tiny
> CL, since it's easy for the change to get lost in a large CL like this one.

Done.

https://codereview.chromium.org/344883002/diff/380001/tools/metrics/histogram...
File tools/metrics/histograms/verify_enum_custom_flags.py (right):

https://codereview.chromium.org/344883002/diff/380001/tools/metrics/histogram...
tools/metrics/histograms/verify_enum_custom_flags.py:236: return self.results
On 2014/08/08 03:49:45, Ilya Sherman wrote:
> On 2014/08/07 23:24:56, alemate wrote:
> > On 2014/08/05 20:14:47, Ilya Sherman wrote:
> > > IMO this presubmit script is not needed now that you're using hashes,
since
> > > there aren't any collisions expected.  The reason that the other approach
> > needed
> > > a presubmit check was that one person accidentally inserting an entry in
the
> > > wrong spot in the enum would ruin the entire data collection process. 
> That's
> > > not a concern with hashes.
> > 
> > Unit test cannot check that values are not removed.
> > So I would better add this check.
> 
> (a) None of the reviewers of this file would be likely to LGTM a CL that
removed
> a switch.
> (b) Nobody editing this file is likely to accidentally remove a switch
> (c) Even if a switch were accidentally removed, it would be easy to restore,
> e.g. using code history.
> (d) No data is corrupted if the histograms.xml file is not up-to-date -- it's
> purely metadata.
> 
> For all of these reasons, I don't think it's worthwhile to have over 200 lines
> of code (plus tests) just to verify that values are not removed.  I strongly
> encourage you to either keep the C++ unittest or to write a complete presubmit
> check; but not both.

Done.

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:7: #include <stdint.h>
On 2014/08/08 03:49:45, Ilya Sherman wrote:
> nit: This is already included in the header file.

Done.

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2277: return base::checked_cast<unsigned>(
On 2014/08/08 03:49:45, Ilya Sherman wrote:
> nit: unsigned -> uint32_t

Done.

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
File chrome/browser/about_flags_histogram_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
chrome/browser/about_flags_histogram_unittest.cc:11: #include
"base/prefs/testing_pref_service.h"*/
On 2014/08/08 03:49:45, Ilya Sherman wrote:
> Please update this.

Done.

https://codereview.chromium.org/344883002/diff/400001/chrome/browser/about_fl...
chrome/browser/about_flags_histogram_unittest.cc:20: //#include
"grit/chromium_strings.h"
On 2014/08/08 03:49:45, Ilya Sherman wrote:
> Please update this.

Done.

sky

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags.cc#newcode2309 chrome/browser/about_flags.cc:2309: VLOG(1) << "ReportCustomFlags(): histogram='" << uma_histogram_hame << "' '" ...

6 years, 4 months ago (2014-08-11 23:45:45 UTC) #96

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags.cc#newcode2309 chrome/browser/about_flags.cc:2309: VLOG(1) << "ReportCustomFlags(): histogram='" << uma_histogram_hame << "' '" ...

6 years, 4 months ago (2014-08-12 18:02:26 UTC) #97

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags.cc#newcode2309 chrome/browser/about_flags.cc:2309: VLOG(1) << "ReportCustomFlags(): histogram='" << uma_histogram_hame << "' '" ...

6 years, 4 months ago (2014-08-12 18:03:15 UTC) #98

Ilya Sherman

Whoops, wrote a comment yesterday but failed to mail it: https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags_histogram_unittest.cc File chrome/browser/about_flags_histogram_unittest.cc (right): https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags_histogram_unittest.cc#newcode1 ...

6 years, 4 months ago (2014-08-12 18:05:31 UTC) #99

sky

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags.cc#newcode2309 chrome/browser/about_flags.cc:2309: VLOG(1) << "ReportCustomFlags(): histogram='" << uma_histogram_hame << "' '" ...

6 years, 4 months ago (2014-08-12 19:57:13 UTC) #100

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2309: VLOG(1) << "ReportCustomFlags():
histogram='" << uma_histogram_hame << "' '"
On 2014/08/12 18:03:15, alemate wrote:
> On 2014/08/12 18:02:25, alemate wrote:
> > On 2014/08/11 23:45:44, sky wrote:
> > > Is there a reason you want VLOG here and not DVLOG?
> > 
> > The idea is see the "reason to restart" in verbose log.
> 
> "The idea is to see" ...

Certainly, but why release and not debug? Do you really need this for release
builds?

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
File chrome/browser/about_flags_histogram_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
chrome/browser/about_flags_histogram_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2014/08/12 18:05:30, Ilya Sherman wrote:
> On 2014/08/11 23:45:45, sky wrote:
> > 2014. But is there a reason this is a separate file and not part of the the
> > existing unittest?
> 
> FWIW, I recommended moving this out to a separate file, as the test uses a
> separate test fixture.  If you'd prefer that it be moved back, I'm ok with
being
> overruled :)

AFAIK the common pattern is to put them in the same file. Plenty of files use
different fixtures.

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
chrome/browser/about_flags_histogram_unittest.cc:216: histograms_xml_file_path =
histograms_xml_file_path.AppendASCII("tools")
On 2014/08/12 18:02:26, alemate wrote:
> On 2014/08/11 23:45:45, sky wrote:
> > I believe this is going to require updating isolate files for
> > swarming.
> 
> This CL includes a change to chrome/unit_tests.isolate . Is it enough?
> 
> > Also, does adding this as a dependency for the test work?
> > If not, you'll need to
> > update the exclusions list for the analyze step. See
> >
>
https://sites.google.com/a/chromium.org/dev/developers/testing/commit-queue/c...
> > for details.
> 
> How can I add dependency for AboutFlagsHistogramTest test?
> As far as I understand your URL, it says : "If you have added test
> dependency and it doesn't work, add your file to exclusions" . But
> how do I add the dependency? And how do I check it works?

If you can express the dependency in the gyp/gypi file and that works as a
dependency, then the analyze step will recognize that if you change
histograms.xml then unit tests must work. I don't know if listing the dependency
as a source will work, but you could try. If it isn't possible, meaning you
can't express the dependency, then add the file to the list of exclusions.

You could check the dependency route by creating a separate patch that adds the
dependency to the appropriate gyp/gypi file, modifies histograms.xml, and modify
the exclusion file by removing the line with ".*gyp[i]?". Now do a try job and
verify unit_tests gets run.

Ilya Sherman

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags_histogram_unittest.cc File chrome/browser/about_flags_histogram_unittest.cc (right): https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags_histogram_unittest.cc#newcode216 chrome/browser/about_flags_histogram_unittest.cc:216: histograms_xml_file_path = histograms_xml_file_path.AppendASCII("tools") On 2014/08/12 19:57:13, sky wrote: > ...

6 years, 4 months ago (2014-08-12 20:07:16 UTC) #101

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
File chrome/browser/about_flags_histogram_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
chrome/browser/about_flags_histogram_unittest.cc:216: histograms_xml_file_path =
histograms_xml_file_path.AppendASCII("tools")
On 2014/08/12 19:57:13, sky wrote:
> On 2014/08/12 18:02:26, alemate wrote:
> > On 2014/08/11 23:45:45, sky wrote:
> > > I believe this is going to require updating isolate files for
> > > swarming.
> > 
> > This CL includes a change to chrome/unit_tests.isolate . Is it enough?
> > 
> > > Also, does adding this as a dependency for the test work?
> > > If not, you'll need to
> > > update the exclusions list for the analyze step. See
> > >
> >
>
https://sites.google.com/a/chromium.org/dev/developers/testing/commit-queue/c...
> > > for details.
> > 
> > How can I add dependency for AboutFlagsHistogramTest test?
> > As far as I understand your URL, it says : "If you have added test
> > dependency and it doesn't work, add your file to exclusions" . But
> > how do I add the dependency? And how do I check it works?
> 
> If you can express the dependency in the gyp/gypi file and that works as a
> dependency, then the analyze step will recognize that if you change
> histograms.xml then unit tests must work. I don't know if listing the
dependency
> as a source will work, but you could try. If it isn't possible, meaning you
> can't express the dependency, then add the file to the list of exclusions.
> 
> You could check the dependency route by creating a separate patch that adds
the
> dependency to the appropriate gyp/gypi file, modifies histograms.xml, and
modify
> the exclusion file by removing the line with ".*gyp[i]?". Now do a try job and
> verify unit_tests gets run.

Oof, I'd really prefer not to have *all* changes to histograms.xml trigger
unit_tests runs.  Is it possible to have a tighter coupling, so that changes to
histograms.xml only trigger this one test to run?  If not, we can use the
sledgehammer approach for now... but if we do, it would be really nice to fix
this in the future.  I routinely submit histograms.xml changes with NOTRY=true,
because it's a huge waste of trybot cycles otherwise.  I'd love to see the
tooling catch up so that I don't need to do this.

sky

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags_histogram_unittest.cc File chrome/browser/about_flags_histogram_unittest.cc (right): https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags_histogram_unittest.cc#newcode216 chrome/browser/about_flags_histogram_unittest.cc:216: histograms_xml_file_path = histograms_xml_file_path.AppendASCII("tools") On 2014/08/12 20:07:16, Ilya Sherman wrote: ...

6 years, 4 months ago (2014-08-12 22:42:03 UTC) #102

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
File chrome/browser/about_flags_histogram_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
chrome/browser/about_flags_histogram_unittest.cc:216: histograms_xml_file_path =
histograms_xml_file_path.AppendASCII("tools")
On 2014/08/12 20:07:16, Ilya Sherman wrote:
> On 2014/08/12 19:57:13, sky wrote:
> > On 2014/08/12 18:02:26, alemate wrote:
> > > On 2014/08/11 23:45:45, sky wrote:
> > > > I believe this is going to require updating isolate files for
> > > > swarming.
> > > 
> > > This CL includes a change to chrome/unit_tests.isolate . Is it enough?
> > > 
> > > > Also, does adding this as a dependency for the test work?
> > > > If not, you'll need to
> > > > update the exclusions list for the analyze step. See
> > > >
> > >
> >
>
https://sites.google.com/a/chromium.org/dev/developers/testing/commit-queue/c...
> > > > for details.
> > > 
> > > How can I add dependency for AboutFlagsHistogramTest test?
> > > As far as I understand your URL, it says : "If you have added test
> > > dependency and it doesn't work, add your file to exclusions" . But
> > > how do I add the dependency? And how do I check it works?
> > 
> > If you can express the dependency in the gyp/gypi file and that works as a
> > dependency, then the analyze step will recognize that if you change
> > histograms.xml then unit tests must work. I don't know if listing the
> dependency
> > as a source will work, but you could try. If it isn't possible, meaning you
> > can't express the dependency, then add the file to the list of exclusions.
> > 
> > You could check the dependency route by creating a separate patch that adds
> the
> > dependency to the appropriate gyp/gypi file, modifies histograms.xml, and
> modify
> > the exclusion file by removing the line with ".*gyp[i]?". Now do a try job
and
> > verify unit_tests gets run.
> 
> Oof, I'd really prefer not to have *all* changes to histograms.xml trigger
> unit_tests runs.  Is it possible to have a tighter coupling, so that changes
to
> histograms.xml only trigger this one test to run?  If not, we can use the
> sledgehammer approach for now... but if we do, it would be really nice to fix
> this in the future.  I routinely submit histograms.xml changes with
NOTRY=true,
> because it's a huge waste of trybot cycles otherwise.  I'd love to see the
> tooling catch up so that I don't need to do this.

We currently only have the sledgehammer:(

Alexander Alekseev

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags.cc#newcode2309 chrome/browser/about_flags.cc:2309: VLOG(1) << "ReportCustomFlags(): histogram='" << uma_histogram_hame << "' '" ...

6 years, 4 months ago (2014-08-13 19:59:34 UTC) #103

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2309: VLOG(1) << "ReportCustomFlags():
histogram='" << uma_histogram_hame << "' '"
On 2014/08/12 19:57:13, sky wrote:
> On 2014/08/12 18:03:15, alemate wrote:
> > On 2014/08/12 18:02:25, alemate wrote:
> > > On 2014/08/11 23:45:44, sky wrote:
> > > > Is there a reason you want VLOG here and not DVLOG?
> > > 
> > > The idea is see the "reason to restart" in verbose log.
> > 
> > "The idea is to see" ...
> 
> Certainly, but why release and not debug? Do you really need this for release
> builds?

Running debug build on device is very slow. So it is usually
"last resort" option.

I moved VLOG() to chromeos-specific code, and replaced if here with DVLOG().

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
File chrome/browser/about_flags_histogram_unittest.cc (right):

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
chrome/browser/about_flags_histogram_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2014/08/12 19:57:13, sky wrote:
> On 2014/08/12 18:05:30, Ilya Sherman wrote:
> > On 2014/08/11 23:45:45, sky wrote:
> > > 2014. But is there a reason this is a separate file and not part of the
the
> > > existing unittest?
> > 
> > FWIW, I recommended moving this out to a separate file, as the test uses a
> > separate test fixture.  If you'd prefer that it be moved back, I'm ok with
> being
> > overruled :)
> 
> AFAIK the common pattern is to put them in the same file. Plenty of files use
> different fixtures.

Done.

https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
chrome/browser/about_flags_histogram_unittest.cc:216: histograms_xml_file_path =
histograms_xml_file_path.AppendASCII("tools")
On 2014/08/12 19:57:13, sky wrote:
> On 2014/08/12 18:02:26, alemate wrote:
> > On 2014/08/11 23:45:45, sky wrote:
> > > I believe this is going to require updating isolate files for
> > > swarming.
> > 
> > This CL includes a change to chrome/unit_tests.isolate . Is it enough?
> > 
> > > Also, does adding this as a dependency for the test work?
> > > If not, you'll need to
> > > update the exclusions list for the analyze step. See
> > >
> >
>
https://sites.google.com/a/chromium.org/dev/developers/testing/commit-queue/c...
> > > for details.
> > 
> > How can I add dependency for AboutFlagsHistogramTest test?
> > As far as I understand your URL, it says : "If you have added test
> > dependency and it doesn't work, add your file to exclusions" . But
> > how do I add the dependency? And how do I check it works?
> 
> If you can express the dependency in the gyp/gypi file and that works as a
> dependency, then the analyze step will recognize that if you change
> histograms.xml then unit tests must work. I don't know if listing the
dependency
> as a source will work, but you could try. If it isn't possible, meaning you
> can't express the dependency, then add the file to the list of exclusions.
> 
> You could check the dependency route by creating a separate patch that adds
the
> dependency to the appropriate gyp/gypi file, modifies histograms.xml, and
modify
> the exclusion file by removing the line with ".*gyp[i]?". Now do a try job and
> verify unit_tests gets run.

It seems that adding histograms.xml to the list of sources works:
https://codereview.chromium.org/470613002/#ps20001
(there is another CL3 with additional changes to exclusions, but it seems it is
enough to add to sources).

sky

On Wed, Aug 13, 2014 at 12:59 PM, <alemate@chromium.org> wrote: > > https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_flags.cc > File ...

6 years, 4 months ago (2014-08-13 20:13:05 UTC) #104

On Wed, Aug 13, 2014 at 12:59 PM,  <alemate@chromium.org> wrote:
>
>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
> File chrome/browser/about_flags.cc (right):
>
>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
> chrome/browser/about_flags.cc:2309: VLOG(1) << "ReportCustomFlags():
> histogram='" << uma_histogram_hame << "' '"
> On 2014/08/12 19:57:13, sky wrote:
>>
>> On 2014/08/12 18:03:15, alemate wrote:
>> > On 2014/08/12 18:02:25, alemate wrote:
>> > > On 2014/08/11 23:45:44, sky wrote:
>> > > > Is there a reason you want VLOG here and not DVLOG?
>> > >
>> > > The idea is see the "reason to restart" in verbose log.
>> >
>> > "The idea is to see" ...
>
>
>> Certainly, but why release and not debug? Do you really need this for
>
> release
>>
>> builds?
>
>
> Running debug build on device is very slow. So it is usually
> "last resort" option.
>
> I moved VLOG() to chromeos-specific code, and replaced if here with
> DVLOG().
>
>
>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
> File chrome/browser/about_flags_histogram_unittest.cc (right):
>
>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
> chrome/browser/about_flags_histogram_unittest.cc:1: // Copyright (c)
> 2011 The Chromium Authors. All rights reserved.
> On 2014/08/12 19:57:13, sky wrote:
>>
>> On 2014/08/12 18:05:30, Ilya Sherman wrote:
>> > On 2014/08/11 23:45:45, sky wrote:
>> > > 2014. But is there a reason this is a separate file and not part
>
> of the the
>>
>> > > existing unittest?
>> >
>> > FWIW, I recommended moving this out to a separate file, as the test
>
> uses a
>>
>> > separate test fixture.  If you'd prefer that it be moved back, I'm
>
> ok with
>>
>> being
>> > overruled :)
>
>
>> AFAIK the common pattern is to put them in the same file. Plenty of
>
> files use
>>
>> different fixtures.
>
>
> Done.
>
>
>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
> chrome/browser/about_flags_histogram_unittest.cc:216:
> histograms_xml_file_path = histograms_xml_file_path.AppendASCII("tools")
> On 2014/08/12 19:57:13, sky wrote:
>>
>> On 2014/08/12 18:02:26, alemate wrote:
>> > On 2014/08/11 23:45:45, sky wrote:
>> > > I believe this is going to require updating isolate files for
>> > > swarming.
>> >
>> > This CL includes a change to chrome/unit_tests.isolate . Is it
>
> enough?
>>
>> >
>> > > Also, does adding this as a dependency for the test work?
>> > > If not, you'll need to
>> > > update the exclusions list for the analyze step. See
>> > >
>> >
>
>
>
https://sites.google.com/a/chromium.org/dev/developers/testing/commit-queue/c...
>>
>> > > for details.
>> >
>> > How can I add dependency for AboutFlagsHistogramTest test?
>> > As far as I understand your URL, it says : "If you have added test
>> > dependency and it doesn't work, add your file to exclusions" . But
>> > how do I add the dependency? And how do I check it works?
>
>
>> If you can express the dependency in the gyp/gypi file and that works
>
> as a
>>
>> dependency, then the analyze step will recognize that if you change
>> histograms.xml then unit tests must work. I don't know if listing the
>
> dependency
>>
>> as a source will work, but you could try. If it isn't possible,
>
> meaning you
>>
>> can't express the dependency, then add the file to the list of
>
> exclusions.
>
>> You could check the dependency route by creating a separate patch that
>
> adds the
>>
>> dependency to the appropriate gyp/gypi file, modifies histograms.xml,
>
> and modify
>>
>> the exclusion file by removing the line with ".*gyp[i]?". Now do a try
>
> job and
>>
>> verify unit_tests gets run.
>
>
> It seems that adding histograms.xml to the list of sources works:
> https://codereview.chromium.org/470613002/#ps20001
> (there is another CL3 with additional changes to exclusions, but it
> seems it is enough to add to sources).

That test wasn't conclusive. You need to remove 'testing/buildbot/.*'
too. Sorry for not realizing that.

Also, could you add a comment as to why histograms.xml as listed as a
source. It's not readily obvious why.

  -Scott

>
> https://codereview.chromium.org/344883002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

sky

On Wed, Aug 13, 2014 at 1:13 PM, Scott Violet <sky@chromium.org> wrote: > On Wed, ...

6 years, 4 months ago (2014-08-13 20:14:28 UTC) #105

On Wed, Aug 13, 2014 at 1:13 PM, Scott Violet <sky@chromium.org> wrote:
> On Wed, Aug 13, 2014 at 12:59 PM,  <alemate@chromium.org> wrote:
>>
>>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
>> File chrome/browser/about_flags.cc (right):
>>
>>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
>> chrome/browser/about_flags.cc:2309: VLOG(1) << "ReportCustomFlags():
>> histogram='" << uma_histogram_hame << "' '"
>> On 2014/08/12 19:57:13, sky wrote:
>>>
>>> On 2014/08/12 18:03:15, alemate wrote:
>>> > On 2014/08/12 18:02:25, alemate wrote:
>>> > > On 2014/08/11 23:45:44, sky wrote:
>>> > > > Is there a reason you want VLOG here and not DVLOG?
>>> > >
>>> > > The idea is see the "reason to restart" in verbose log.
>>> >
>>> > "The idea is to see" ...
>>
>>
>>> Certainly, but why release and not debug? Do you really need this for
>>
>> release
>>>
>>> builds?
>>
>>
>> Running debug build on device is very slow. So it is usually
>> "last resort" option.
>>
>> I moved VLOG() to chromeos-specific code, and replaced if here with
>> DVLOG().
>>
>>
>>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
>> File chrome/browser/about_flags_histogram_unittest.cc (right):
>>
>>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
>> chrome/browser/about_flags_histogram_unittest.cc:1: // Copyright (c)
>> 2011 The Chromium Authors. All rights reserved.
>> On 2014/08/12 19:57:13, sky wrote:
>>>
>>> On 2014/08/12 18:05:30, Ilya Sherman wrote:
>>> > On 2014/08/11 23:45:45, sky wrote:
>>> > > 2014. But is there a reason this is a separate file and not part
>>
>> of the the
>>>
>>> > > existing unittest?
>>> >
>>> > FWIW, I recommended moving this out to a separate file, as the test
>>
>> uses a
>>>
>>> > separate test fixture.  If you'd prefer that it be moved back, I'm
>>
>> ok with
>>>
>>> being
>>> > overruled :)
>>
>>
>>> AFAIK the common pattern is to put them in the same file. Plenty of
>>
>> files use
>>>
>>> different fixtures.
>>
>>
>> Done.
>>
>>
>>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
>> chrome/browser/about_flags_histogram_unittest.cc:216:
>> histograms_xml_file_path = histograms_xml_file_path.AppendASCII("tools")
>> On 2014/08/12 19:57:13, sky wrote:
>>>
>>> On 2014/08/12 18:02:26, alemate wrote:
>>> > On 2014/08/11 23:45:45, sky wrote:
>>> > > I believe this is going to require updating isolate files for
>>> > > swarming.
>>> >
>>> > This CL includes a change to chrome/unit_tests.isolate . Is it
>>
>> enough?
>>>
>>> >
>>> > > Also, does adding this as a dependency for the test work?
>>> > > If not, you'll need to
>>> > > update the exclusions list for the analyze step. See
>>> > >
>>> >
>>
>>
>>
https://sites.google.com/a/chromium.org/dev/developers/testing/commit-queue/c...
>>>
>>> > > for details.
>>> >
>>> > How can I add dependency for AboutFlagsHistogramTest test?
>>> > As far as I understand your URL, it says : "If you have added test
>>> > dependency and it doesn't work, add your file to exclusions" . But
>>> > how do I add the dependency? And how do I check it works?
>>
>>
>>> If you can express the dependency in the gyp/gypi file and that works
>>
>> as a
>>>
>>> dependency, then the analyze step will recognize that if you change
>>> histograms.xml then unit tests must work. I don't know if listing the
>>
>> dependency
>>>
>>> as a source will work, but you could try. If it isn't possible,
>>
>> meaning you
>>>
>>> can't express the dependency, then add the file to the list of
>>
>> exclusions.
>>
>>> You could check the dependency route by creating a separate patch that
>>
>> adds the
>>>
>>> dependency to the appropriate gyp/gypi file, modifies histograms.xml,
>>
>> and modify
>>>
>>> the exclusion file by removing the line with ".*gyp[i]?". Now do a try
>>
>> job and
>>>
>>> verify unit_tests gets run.
>>
>>
>> It seems that adding histograms.xml to the list of sources works:
>> https://codereview.chromium.org/470613002/#ps20001
>> (there is another CL3 with additional changes to exclusions, but it
>> seems it is enough to add to sources).
>
> That test wasn't conclusive. You need to remove 'testing/buildbot/.*'
> too. Sorry for not realizing that.

Also, you can tell this happened because the analyze step produced this output:

http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...

testing/buildbot/trybot_analyze_config.json (regex = 'testing/buildbot/.*')

which means one of the files matched an exclusion rule and so
everything is going to be built and run.

   -Scott


>
> Also, could you add a comment as to why histograms.xml as listed as a
> source. It's not readily obvious why.
>
>   -Scott
>
>>
>> https://codereview.chromium.org/344883002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Alexander Alekseev

On 2014/08/13 20:14:28, sky wrote: > On Wed, Aug 13, 2014 at 1:13 PM, Scott ...

6 years, 4 months ago (2014-08-14 02:10:06 UTC) #106

On 2014/08/13 20:14:28, sky wrote:
> On Wed, Aug 13, 2014 at 1:13 PM, Scott Violet <mailto:sky@chromium.org> wrote:
> > On Wed, Aug 13, 2014 at 12:59 PM,  <mailto:alemate@chromium.org> wrote:
> >>
> >>
>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
> >> File chrome/browser/about_flags.cc (right):
> >>
> >>
>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
> >> chrome/browser/about_flags.cc:2309: VLOG(1) << "ReportCustomFlags():
> >> histogram='" << uma_histogram_hame << "' '"
> >> On 2014/08/12 19:57:13, sky wrote:
> >>>
> >>> On 2014/08/12 18:03:15, alemate wrote:
> >>> > On 2014/08/12 18:02:25, alemate wrote:
> >>> > > On 2014/08/11 23:45:44, sky wrote:
> >>> > > > Is there a reason you want VLOG here and not DVLOG?
> >>> > >
> >>> > > The idea is see the "reason to restart" in verbose log.
> >>> >
> >>> > "The idea is to see" ...
> >>
> >>
> >>> Certainly, but why release and not debug? Do you really need this for
> >>
> >> release
> >>>
> >>> builds?
> >>
> >>
> >> Running debug build on device is very slow. So it is usually
> >> "last resort" option.
> >>
> >> I moved VLOG() to chromeos-specific code, and replaced if here with
> >> DVLOG().
> >>
> >>
> >>
>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
> >> File chrome/browser/about_flags_histogram_unittest.cc (right):
> >>
> >>
>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
> >> chrome/browser/about_flags_histogram_unittest.cc:1: // Copyright (c)
> >> 2011 The Chromium Authors. All rights reserved.
> >> On 2014/08/12 19:57:13, sky wrote:
> >>>
> >>> On 2014/08/12 18:05:30, Ilya Sherman wrote:
> >>> > On 2014/08/11 23:45:45, sky wrote:
> >>> > > 2014. But is there a reason this is a separate file and not part
> >>
> >> of the the
> >>>
> >>> > > existing unittest?
> >>> >
> >>> > FWIW, I recommended moving this out to a separate file, as the test
> >>
> >> uses a
> >>>
> >>> > separate test fixture.  If you'd prefer that it be moved back, I'm
> >>
> >> ok with
> >>>
> >>> being
> >>> > overruled :)
> >>
> >>
> >>> AFAIK the common pattern is to put them in the same file. Plenty of
> >>
> >> files use
> >>>
> >>> different fixtures.
> >>
> >>
> >> Done.
> >>
> >>
> >>
>
https://codereview.chromium.org/344883002/diff/420001/chrome/browser/about_fl...
> >> chrome/browser/about_flags_histogram_unittest.cc:216:
> >> histograms_xml_file_path = histograms_xml_file_path.AppendASCII("tools")
> >> On 2014/08/12 19:57:13, sky wrote:
> >>>
> >>> On 2014/08/12 18:02:26, alemate wrote:
> >>> > On 2014/08/11 23:45:45, sky wrote:
> >>> > > I believe this is going to require updating isolate files for
> >>> > > swarming.
> >>> >
> >>> > This CL includes a change to chrome/unit_tests.isolate . Is it
> >>
> >> enough?
> >>>
> >>> >
> >>> > > Also, does adding this as a dependency for the test work?
> >>> > > If not, you'll need to
> >>> > > update the exclusions list for the analyze step. See
> >>> > >
> >>> >
> >>
> >>
> >>
>
https://sites.google.com/a/chromium.org/dev/developers/testing/commit-queue/c...
> >>>
> >>> > > for details.
> >>> >
> >>> > How can I add dependency for AboutFlagsHistogramTest test?
> >>> > As far as I understand your URL, it says : "If you have added test
> >>> > dependency and it doesn't work, add your file to exclusions" . But
> >>> > how do I add the dependency? And how do I check it works?
> >>
> >>
> >>> If you can express the dependency in the gyp/gypi file and that works
> >>
> >> as a
> >>>
> >>> dependency, then the analyze step will recognize that if you change
> >>> histograms.xml then unit tests must work. I don't know if listing the
> >>
> >> dependency
> >>>
> >>> as a source will work, but you could try. If it isn't possible,
> >>
> >> meaning you
> >>>
> >>> can't express the dependency, then add the file to the list of
> >>
> >> exclusions.
> >>
> >>> You could check the dependency route by creating a separate patch that
> >>
> >> adds the
> >>>
> >>> dependency to the appropriate gyp/gypi file, modifies histograms.xml,
> >>
> >> and modify
> >>>
> >>> the exclusion file by removing the line with ".*gyp[i]?". Now do a try
> >>
> >> job and
> >>>
> >>> verify unit_tests gets run.
> >>
> >>
> >> It seems that adding histograms.xml to the list of sources works:
> >> https://codereview.chromium.org/470613002/#ps20001
> >> (there is another CL3 with additional changes to exclusions, but it
> >> seems it is enough to add to sources).
> >
> > That test wasn't conclusive. You need to remove 'testing/buildbot/.*'
> > too. Sorry for not realizing that.
> 
> Also, you can tell this happened because the analyze step produced this
output:
> 
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...
> 
> testing/buildbot/trybot_analyze_config.json (regex = 'testing/buildbot/.*')
> 
> which means one of the files matched an exclusion rule and so
> everything is going to be built and run.

Yes, you are right. histograms.xml has to be added to exceptions.
Also I've added a comment in chrome/chrome_tests_unit.gypi on histograms.xml
listed in sources.

PTAL.

Alexander Alekseev

The CQ bit was checked by alemate@chromium.org

6 years, 4 months ago (2014-08-14 17:02:31 UTC) #108

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/344883002/560001

6 years, 4 months ago (2014-08-14 17:04:35 UTC) #109

commit-bot: I haz the power

FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux ...

6 years, 4 months ago (2014-08-15 00:17:23 UTC) #110

commit-bot: I haz the power

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

6 years, 4 months ago (2014-08-15 06:18:08 UTC) #111

commit-bot: I haz the power

Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/builds/6074)

6 years, 4 months ago (2014-08-15 06:18:11 UTC) #112

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/344883002/560001

6 years, 4 months ago (2014-08-15 09:08:26 UTC) #114

commit-bot: I haz the power

FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux ...

6 years, 4 months ago (2014-08-15 09:15:51 UTC) #115

commit-bot: I haz the power

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

6 years, 4 months ago (2014-08-15 09:25:43 UTC) #116

commit-bot: I haz the power

Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/builds/6388)

6 years, 4 months ago (2014-08-15 09:25:45 UTC) #117

Nikita (slow)

Rebase is needed: Applying the patch. Failed to apply patch for chrome/browser/about_flags.cc:

6 years, 4 months ago (2014-08-15 09:26:34 UTC) #118

Alexander Alekseev

The CQ bit was checked by alemate@chromium.org

6 years, 4 months ago (2014-08-15 14:29:38 UTC) #119

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/344883002/560001

6 years, 4 months ago (2014-08-15 14:31:38 UTC) #120

Alexander Alekseev

The CQ bit was unchecked by alemate@chromium.org

6 years, 4 months ago (2014-08-15 14:33:07 UTC) #121

Alexander Alekseev

The CQ bit was checked by alemate@chromium.org

6 years, 4 months ago (2014-08-15 14:41:04 UTC) #122

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/344883002/580001

6 years, 4 months ago (2014-08-15 14:42:21 UTC) #123

commit-bot: I haz the power

FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux ...

6 years, 4 months ago (2014-08-15 16:36:24 UTC) #124

commit-bot: I haz the power

Committed patchset #30 (580001) as 289919

6 years, 4 months ago (2014-08-15 17:13:50 UTC) #125

chromium-reviews

I noticed that we see a bunch of hashes which the dashboard isn't able to ...

6 years, 4 months ago (2014-08-22 15:05:25 UTC) #126

chromium-reviews

Also, a lot of the unmapped ones above seem to have negative values, while the ...

6 years, 4 months ago (2014-08-22 15:07:55 UTC) #127

chromium-reviews

I'll look into it. (During the last 4 days git migration happened and I am ...

6 years, 3 months ago (2014-08-26 11:55:24 UTC) #128

chromium-reviews

6 years, 3 months ago (2014-08-27 17:15:04 UTC) #129

Actually CustomFlags histogram is reported as full uint32_t .
On the other hand, most chromeOS devices are 32-bit, so locally those
integers are negative.
Negative integers in uma.googleplex.com means that histogram values are
actually signed and have different repsesentation on 32 oand 64-bit
systems. That's sad.
I'll make the fix for it.

On Fri, Aug 22, 2014 at 7:07 PM, Alexei Svitkine <asvitkine@google.com>
wrote:

> Also, a lot of the unmapped ones above seem to have negative values, while
> the XML file doesn't have any with negative values. Perhaps there's an
> issue with signed/unsigned conversion? (I didn't check your CL, but my
> impression was that the plan was to always send positive hashes - i.e.
> strip the sign bit - perhaps the C++ code doesn't do that while the code
> that generates the XML definition does?)
>
>
> On Fri, Aug 22, 2014 at 11:04 AM, Alexei Svitkine <asvitkine@google.com>
> wrote:
>
>> I noticed that we see a bunch of hashes which the dashboard isn't able to
>> map to flags with this histogram:
>>
>>
>>
https://uma.googleplex.com/p/chrome/histograms/?dayCount=1&endDate=08-21-2014...
>>
>> Any idea where those are coming from?
>>
>> Could it be that we're missing some flags (e.g. those defined in
>> components or elsewhere)? Could you investigate making the mapping more
>> comprehensive? Thanks.
>>
>>
>> On Fri, Aug 15, 2014 at 1:13 PM, <commit-bot@chromium.org> wrote:
>>
>>> Committed patchset #30 (580001) as 289919
>>>
>>> https://codereview.chromium.org/344883002/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Issue 344883002: Collect UMA statistics on which chrome://flags lead to chrome restart on ChromeOS. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix bug in about_flags.cc. Sdded comments. s/restarting/custom/ #

Patch Set 3 : Add switches histogram IDs to about_flags. #

Patch Set 4 : Added test for AreSwitchesIdenticalToCurrentCommandLine(..., difference). #

Patch Set 5 : Update after review. #

Patch Set 6 : Update after review. #

Patch Set 7 : Added myself as OWNER of about_flags_switches_histogram_ids.h #

Patch Set 8 : Remove "enable-accessibility-script-injection" from about_flags.cc #

Patch Set 9 : Remove unused UMA_HISTOGRAM_ID_kEnableAccessibilityScriptInjection. #

Patch Set 10 : Move ReportCustomFlags to about_flags.cc #

Patch Set 11 : Update after review. #

Patch Set 12 : Renamed about_flags_switches_histogram_id.h -> about_flags_switches_histogram_ids.h . #

Patch Set 13 : Presubmit hook added. #

Patch Set 14 : Removed presubmit hooks, refactored unit test, fixed isolation. #

Patch Set 15 : Use UMA_HISTOGRAM_SPARSE_SLOWLY macro. #

Patch Set 16 : Use PathService to get DIR_SOURCE_ROOT in unit test. #

Patch Set 17 : Update after review. #

Patch Set 18 : Update after review. #

Patch Set 19 : Report hash(switch name). #

Patch Set 20 : Update after review. #

Patch Set 21 : Update after review. #

Patch Set 22 : Update after review. #

Patch Set 23 : Comment updated. #

Patch Set 24 : Merge tests. Add test dependency on histograms.xml. #

Patch Set 25 : Added a comment why histograms.xml is listed in the sources. #

Patch Set 26 : Rebased. #

Patch Set 27 : Add exception for histograms.xml. #

Patch Set 28 : Added comments to about_flags.cc. Added missing histogram enum entry. #

Patch Set 29 : Fix windows build. #

Patch Set 30 : Rebased. #

Messages