Improvements to incognito and file access metrics for extensions: only record

Improvements to incognito and file access metrics for extensions: only record
if there is at least one extension that can be allowed for each, and exclude
policy and unpacked extensions.

BUG=372047
R=jar@chromium.org, rdevlin.cronin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272198

[Devlin, 2014-05-19 22:10:01]

lgtm

https://codereview.chromium.org/297533002/diff/1/chrome/browser/extensions/in...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/297533002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:448:
!Manifest::IsUnpackedLocation(extension->location()) &&
nit: You don't need the IsUnpacked check here, since it's done on line 368. That
said, if you want to keep it, I'm not totally opposed, as it makes it more
clear, and ensures this stays valid if we change something above (and it's a
really fast check).

[not at google - send to devlin, 2014-05-19 22:15:23]

https://codereview.chromium.org/297533002/diff/1/chrome/browser/extensions/in...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/297533002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:448:
!Manifest::IsUnpackedLocation(extension->location()) &&
On 2014/05/19 22:10:01, D Cronin wrote:
> nit: You don't need the IsUnpacked check here, since it's done on line 368.
That
> said, if you want to keep it, I'm not totally opposed, as it makes it more
> clear, and ensures this stays valid if we change something above (and it's a
> really fast check).

ah yes. when will I learn. fixed.

[not at google - send to devlin, 2014-05-20 16:11:09]

ping @ jar

[jar (doing other things), 2014-05-20 17:43:33]

I was surprised to see a total of 82 distinct histograms in the enclosed file
("surprised" meaning "that is a lot of distinct histograms!"). Histograms are
fast, but if you make enough of them... the memory cost will start to show.

I took a look at them, and could see that several were also super wasteful, by
about a needless factor of 10.  I made suggestions for the ones that stood out
(enumerated histograms where a constant of 100 was used, rather than the actual
value... which was generally under 10).

Please consider whether you really need, or have any plans for using these
histograms.  IF you do, then fine... but think about it. A 100 bucket enumerated
histograms] probably costs about 1KB of allocated storage... and with 82 of
them, you're going to start to be noticeable.  Truth is, many are "only" 50
buckets, or less... but don't just toss in more histograms, or finer granularity
of breakdowns, if you have no reason to look at the data (and it sure seems like
you have a lot of histograms).

I've also added asvitkine as a review going forward.

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:226: reload_reason, 100);
This should use a precise top value + 1, rather than 100.

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:319:
UMA_HISTOGRAM_ENUMERATION("Extensions.AppLocation", location, 100);
All of these histograms should have the more precise enumeration size, rather
than "100," such as:

UMA_HISTOGRAM_ENUMERATION("Extensions.AppLocation", location,
Manifest::NUM_LOCATIONS)

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:337:
"Extensions.FromWebstoreInconsistency", BAD_UPDATE_URL, 2);
This uses a histogram as a counter, as I can't find any other contributors to
this histogram... and the sample BAD_UPDATE_URL appears to be a constant.

Looking at the dashboard, I see a second value... but can't find the code that
might inject it.

IN general, it is better to combine several of these "counter like" histograms
into a single enumerated histogram of related counters, so that you can
normalize the counts for one bucket, relative to others.  In addition, it is way
more efficient to use few histograms.

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:373: 10);
Why did you use 10 here?  Better would be a limit, if it can be calculated.  IF
you really want this to be open ended, you could use a sparse (slow) histogram.

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:387:
UMA_HISTOGRAM_ENUMERATION("Extensions.LoadType", type, 100);
Here again, rather than 100, I think the numeber is closer to 8... but you
should use MAX_VALUE + 1 (but I don't think you defined such in the current Type
enum.

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:532: if (incognito + not_incognito
> 0) {
nit: the above is seemingly meant to be clever, but perhaps clearer would be:
if (incognito > 0 || not_incognito > 0)


This looks like it is changing what is recorded in the histograms.

Are you taking that change into account with your planned(?) use of the values?
If not, you'll get conflated data... and wish you had a new name for the
histogram.

[not at google - send to devlin, 2014-05-20 17:53:09]

I'll internalise your other comments and have a look at fixing them up in
another CL hopefully.

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:532: if (incognito + not_incognito
> 0) {
On 2014/05/20 17:43:34, jar wrote:
> nit: the above is seemingly meant to be clever, but perhaps clearer would be:
> if (incognito > 0 || not_incognito > 0)
> 
> 
> This looks like it is changing what is recorded in the histograms.
> 
> Are you taking that change into account with your planned(?) use of the
values?
> If not, you'll get conflated data... and wish you had a new name for the
> histogram.

We only just added this metric (a few days ago), so I'm ok with that.

[jar (doing other things), 2014-05-20 17:56:39]

On 2014/05/20 17:53:09, kalman wrote:
> I'll internalise your other comments and have a look at fixing them up in
> another CL hopefully.
> 
>
https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
> File chrome/browser/extensions/installed_loader.cc (right):
> 
>
https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
> chrome/browser/extensions/installed_loader.cc:532: if (incognito +
not_incognito
> > 0) {
> On 2014/05/20 17:43:34, jar wrote:
> > nit: the above is seemingly meant to be clever, but perhaps clearer would
be:
> > if (incognito > 0 || not_incognito > 0)
> > 
> > 
> > This looks like it is changing what is recorded in the histograms.
> > 
> > Are you taking that change into account with your planned(?) use of the
> values?
> > If not, you'll get conflated data... and wish you had a new name for the
> > histogram.
> 
> We only just added this metric (a few days ago), so I'm ok with that.

Please correct the readability nit.

[jar (doing other things), 2014-05-20 17:58:16]

All the changes I suggested (to enumerated histograms) are completely backwards
compatible with your existing data (i.e., won't conflat or confuse any data).  

Please correct the upper bounds in these histograms with actual limits.  There
is no reason to be wasteful in that regard.

[not at google - send to devlin, 2014-05-20 18:06:20]

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:532: if (incognito + not_incognito
> 0) {
On 2014/05/20 17:53:10, kalman wrote:
> On 2014/05/20 17:43:34, jar wrote:
> > nit: the above is seemingly meant to be clever, but perhaps clearer would
be:
> > if (incognito > 0 || not_incognito > 0)
> > 
> > 
> > This looks like it is changing what is recorded in the histograms.
> > 
> > Are you taking that change into account with your planned(?) use of the
> values?
> > If not, you'll get conflated data... and wish you had a new name for the
> > histogram.
> 
> We only just added this metric (a few days ago), so I'm ok with that.

I can change it but I think the existing condition and your suggestion are just
as understandable. I read "incognito + not_incognito" as "the number of
extensions that could have been enable in incognito", and this metric isn't
interesting if that's 0. I could add a comment to make that clearer.

[not at google - send to devlin, 2014-05-20 18:15:49]

I've made the change to the condition though IMO it's less readable.

[jar (doing other things), 2014-05-20 23:01:34]

Please fix up the bounds in the enumerated histograms, per earlier comments (on
patch 1).

Thanks!

[not at google - send to devlin, 2014-05-20 23:16:40]

On 2014/05/20 23:01:34, jar wrote:
> Please fix up the bounds in the enumerated histograms, per earlier comments
(on
> patch 1).
> 
> Thanks!

Which bounds check? Most of the code I didn't write nor planned to change in
this CL.

I don't mind fixing up in a follow-up though, you're right that this file is a
mess and often wasteful.

[jar (doing other things), 2014-05-20 23:31:36]

On 2014/05/20 17:43:33, jar wrote:
> I was surprised to see a total of 82 distinct histograms in the enclosed file
> ("surprised" meaning "that is a lot of distinct histograms!"). Histograms are
> fast, but if you make enough of them... the memory cost will start to show.
> 
> I took a look at them, and could see that several were also super wasteful, by
> about a needless factor of 10.  I made suggestions for the ones that stood out
> (enumerated histograms where a constant of 100 was used, rather than the
actual
> value... which was generally under 10).
> 
> Please consider whether you really need, or have any plans for using these
> histograms.  IF you do, then fine... but think about it. A 100 bucket
enumerated
> histograms] probably costs about 1KB of allocated storage... and with 82 of
> them, you're going to start to be noticeable.  Truth is, many are "only" 50
> buckets, or less... but don't just toss in more histograms, or finer
granularity
> of breakdowns, if you have no reason to look at the data (and it sure seems
like
> you have a lot of histograms).
> 
> 
>
https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
> File chrome/browser/extensions/installed_loader.cc (right):
> 
>
https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
> chrome/browser/extensions/installed_loader.cc:226: reload_reason, 100);
> This should use a precise top value + 1, rather than 100.
> 
>
https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
> chrome/browser/extensions/installed_loader.cc:319:
> UMA_HISTOGRAM_ENUMERATION("Extensions.AppLocation", location, 100);
> All of these histograms should have the more precise enumeration size, rather
> than "100," such as:
> 
> UMA_HISTOGRAM_ENUMERATION("Extensions.AppLocation", location,
> Manifest::NUM_LOCATIONS)
> 
>
https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
> chrome/browser/extensions/installed_loader.cc:337:
> "Extensions.FromWebstoreInconsistency", BAD_UPDATE_URL, 2);
> This uses a histogram as a counter, as I can't find any other contributors to
> this histogram... and the sample BAD_UPDATE_URL appears to be a constant.
> 
> Looking at the dashboard, I see a second value... but can't find the code that
> might inject it.
> 
> IN general, it is better to combine several of these "counter like" histograms
> into a single enumerated histogram of related counters, so that you can
> normalize the counts for one bucket, relative to others.  In addition, it is
way
> more efficient to use few histograms.
> 
>
https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
> chrome/browser/extensions/installed_loader.cc:373: 10);
> Why did you use 10 here?  Better would be a limit, if it can be calculated. 
IF
> you really want this to be open ended, you could use a sparse (slow)
histogram.
> 
>
https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
> chrome/browser/extensions/installed_loader.cc:387:
> UMA_HISTOGRAM_ENUMERATION("Extensions.LoadType", type, 100);
> Here again, rather than 100, I think the numeber is closer to 8... but you
> should use MAX_VALUE + 1 (but I don't think you defined such in the current
Type
> enum.
> 


The above were my comments about the enumerated histograms that I referred to in
my last review.  The "bounding" argument is the third argument to
UMA_HISTOGRAM_ENUMERATION().  It should be one greater than any samples that are
ever submitted to the histogram macro.

In all places where a random constant (like 100) is placed, please change it to
use a special final member of the C++ enum that is supplying the range of
samples.  I gave one example where there was already such a enumerated constant
defined, and I think it would be simple enough to add such a constant in the
other one(s?).  Just search the file for UMA_HISTOGRAM_ENUMERATION() and be sure
the last argument is the correct value (and certainly not a 10 or a 100).

Since you were modifying histograms in this file, it seemed good to clean up the
most apparent top-level waste among them.  Yeah... I know you didn't write this
stuff in the first place....

Thanks in advance!

[not at google - send to devlin, 2014-05-20 23:35:29]

> Since you were modifying histograms in this file, it seemed good to clean up
the
> most apparent top-level waste among them.  Yeah... I know you didn't write
this
> stuff in the first place....

It's good to clean up but not in this CL which is targeted at a specific issue
with a specific histogram.

[jar (doing other things), 2014-05-21 00:27:20]

You mentioned that this "CL ... is targeted at a specific issue...".  Is the
point that this is a "targeted" landing of a minimal CL, that you're hoping to
merge to other branches, and is that why you're not interested in fixing up the
adjacent questionable code?

If so, please file a bug, and add a todo inline with your email address and bug
number.  

IMO, the code changes are small enough, and should cause no conflict, and would
be better simply landed here, even if you planned on a merge.

Thanks.

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:532: if (incognito + not_incognito
> 0) {
If you're really enamoured with using the sum, you could rename the variables to
something like:

extension_count_for_incognito
extension_count_for_non_incognito

Adding those two named variables would then make sense, as they are both counts
(and hence non-negative), and it would be clearer why adding produced an
interesting sum (the positive sum clearly indicates if there are some
extensions).  

The current variable names made this less clear, and almost sounded more like
booleans (and their declaration gave no hint... I had to read code to find their
meanings).

[not at google - send to devlin, 2014-05-21 01:38:48]

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:532: if (incognito + not_incognito
> 0) {
On 2014/05/21 00:27:20, jar wrote:
> If you're really enamoured with using the sum, you could rename the variables
to
> something like:
> 
> extension_count_for_incognito
> extension_count_for_non_incognito
> 
> Adding those two named variables would then make sense, as they are both
counts
> (and hence non-negative), and it would be clearer why adding produced an
> interesting sum (the positive sum clearly indicates if there are some
> extensions).  
> 
> The current variable names made this less clear, and almost sounded more like
> booleans (and their declaration gave no hint... I had to read code to find
their
> meanings).
> 
> 

That change SGTM. I'll come back to this CL tomorrow.

[not at google - send to devlin, 2014-05-21 01:45:34]

On 2014/05/21 00:27:20, jar wrote:
> You mentioned that this "CL ... is targeted at a specific issue...".  Is the
> point that this is a "targeted" landing of a minimal CL, that you're hoping to
> merge to other branches, and is that why you're not interested in fixing up
the
> adjacent questionable code?
> 
> If so, please file a bug, and add a todo inline with your email address and
bug
> number.  
> 
> IMO, the code changes are small enough, and should cause no conflict, and
would
> be better simply landed here, even if you planned on a merge.
> 
> Thanks.

I'm not hoping to merge this, but that's not the only reason to keep this change
within its current scope. Other reasons:
* change history looks less random.
* patch can be more easily reverted.
* patch has less of a chance of being reverted due to some unforseen issue with
other changes.
* I don't need Devlin to have a look at the changes.
* less work for me right now.
* I can move on and do the follow-up without worrying about time pressure or
code rot.

The changes aren't totally trivial. For example, adding more enum values will
cause compile errors elsewhere in the codebase where there are switches with no
default case (which is a good thing, precisely why they don't have a default
case). I'm not speculating; I actually had a go at the enum changes a little
while ago. Indeed I can fix those up easily enough, but it'll extend the change
across more files.

To be clear, I'm not "not interested in fixing up the adjacent questionable
code", I will happily do it in a follow up. I can file a bug for that.

[not at google - send to devlin, 2014-05-21 19:46:43]

I've responded to your previous comments, but the updates are in this CL:
https://codereview.chromium.org/299853002/

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:226: reload_reason, 100);
On 2014/05/20 17:43:34, jar wrote:
> This should use a precise top value + 1, rather than 100.

Done.

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:319:
UMA_HISTOGRAM_ENUMERATION("Extensions.AppLocation", location, 100);
On 2014/05/20 17:43:34, jar wrote:
> All of these histograms should have the more precise enumeration size, rather
> than "100," such as:
> 
> UMA_HISTOGRAM_ENUMERATION("Extensions.AppLocation", location,
> Manifest::NUM_LOCATIONS)

Done.

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:337:
"Extensions.FromWebstoreInconsistency", BAD_UPDATE_URL, 2);
On 2014/05/20 17:43:34, jar wrote:
> This uses a histogram as a counter, as I can't find any other contributors to
> this histogram... and the sample BAD_UPDATE_URL appears to be a constant.
> 
> Looking at the dashboard, I see a second value... but can't find the code that
> might inject it.
> 
> IN general, it is better to combine several of these "counter like" histograms
> into a single enumerated histogram of related counters, so that you can
> normalize the counts for one bucket, relative to others.  In addition, it is
way
> more efficient to use few histograms.

yeah I have no idea what this histogram is supposed to be doing

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:373: 10);
On 2014/05/20 17:43:34, jar wrote:
> Why did you use 10 here?  Better would be a limit, if it can be calculated. 
IF
> you really want this to be open ended, you could use a sparse (slow)
histogram.

This is basically going to be either 1 or 2 at the moment (unless extensions
give it some invalid value), and will grow very slowly over time (maybe
increment by 1 every 1-2 years). I could define the "2" as a constant somewhere
but I'm worried it'll be lost in the wash in the future (currently we don't
assume *anything* about the maximum value of the manifest version).

what is a sparse histogram?

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:387:
UMA_HISTOGRAM_ENUMERATION("Extensions.LoadType", type, 100);
On 2014/05/20 17:43:34, jar wrote:
> Here again, rather than 100, I think the numeber is closer to 8... but you
> should use MAX_VALUE + 1 (but I don't think you defined such in the current
Type
> enum.

Done.

[not at google - send to devlin, 2014-05-21 23:41:03]

PTAL.

[jar (doing other things), 2014-05-22 02:19:06]

lgtm

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/297533002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:373: 10);
On 2014/05/21 19:46:43, kalman wrote:
> 
> what is a sparse histogram?

Most histograms are implemented (under the covers) as vectors, where each sample
merely increments one of the elements (the appropriate bucket in the histogram).
The code is extremely fast, allows multithreaded access, and does not (both to)
use locks.  It is more likely that the user will reboot his machine, and add
noise to a bucket count, than it is that a race will corrupt the bucket into an
off-by-one error ;-). 

A sparse histogram is implemented as a map, and hence can take any values.  It
is especially helpful when trying to histogram (for example) OS error codes.  We
certainly don't want to pre-allocate a vector of length 64K for such.... and it
is rare to have any entries at all.  As a result, we use a map... but then we
need <sigh> to use a lock to ensure we don't call malloc and grow the map while
someone else adds a sample (note that a pre-allocated vector doesn't move..
ever!).  The result is a memory efficient histogram of a very sparse sample
set... but it runs slowly (relatively speaking) when compared with the blazing
speed of standard histogram sample recording.

As long as you're rarely calling for a sample in a sparse histogram, or the
activity to gather the data takes a long time anyway, a sparse histogram may be
a good alternative.

[not at google - send to devlin, 2014-05-22 15:48:28]

Committed patchset #6 manually as r272198 (presubmit successful).