Additional metrics for disabled extensions and content verification

Additional metrics for disabled extensions and content verification


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

[asargent_no_longer_on_chrome, 2014-07-23 21:53:25]

isherman: histograms usage and histograms.xml
rockot: extensions code

Thanks!

[Ilya Sherman, 2014-07-23 21:57:11]

+Jesse for histograms review

[Ken Rockot(use gerrit already), 2014-07-24 03:56:02]

https://codereview.chromium.org/412003003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/412003003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_system_impl.cc:240: } else if
(!ContainsKey(would_be_disabled_ids_, extension_id)) {
Your choice between readability vs efficiency here, but you could also:

} else if (would_be_disabled_ids_.insert(extension_id).second) {
  UMA_HISTOGRAM(...);
}

to get the test and insert in one operation.

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

https://codereview.chromium.org/412003003/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:137: // Records the diable reasons
for a single extension grouped by
nit: disable

https://codereview.chromium.org/412003003/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:144: reason = (1 << ++shift)) {
What about:

for (int reason = 1; reason < ...; reason <<= 1) {

instead?

https://codereview.chromium.org/412003003/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:145: if ((reason == 0 && reasons
== 0) || reasons & reason)
OK now I'm confused; I don't think reason can ever be 0 here.

Here's my guess for what the whole function should be, if I understand the
intent correctly:

void RecordDisableReasons(int reasons) {
  if (reasons == Extension::DISABLE_NONE) {
    UMA_HISTOGRAM_ENUMERATION("Extensions.DisableReason",
Extension::DISABLE_NONE, Extension::DISABLE_REASON_LAST);
    return;
  }
  for (int reason = 1; reason < Extension::DISABLE_REASON_LAST; reason <<= 1) {
    if (reasons & reason) {
      UMA_HISTOGRAM_ENUMERATION("Extensions.DisableReason", reason,
Extension::DISABLE_REASON_LAST);
    }
  }
}

[asargent_no_longer_on_chrome, 2014-07-24 17:08:12]

https://codereview.chromium.org/412003003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/412003003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_system_impl.cc:240: } else if
(!ContainsKey(would_be_disabled_ids_, extension_id)) {
On 2014/07/24 03:56:01, Ken Rockot wrote:
> Your choice between readability vs efficiency here, but you could also:
> 
> } else if (would_be_disabled_ids_.insert(extension_id).second) {
>   UMA_HISTOGRAM(...);
> }
> 
> to get the test and insert in one operation.

I'm going to pick readability, since this won't be called very frequently and
even when it is, the insert only happens once for a given id.

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

https://codereview.chromium.org/412003003/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:137: // Records the diable reasons
for a single extension grouped by
On 2014/07/24 03:56:02, Ken Rockot wrote:
> nit: disable

Done.

https://codereview.chromium.org/412003003/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:144: reason = (1 << ++shift)) {
On 2014/07/24 03:56:02, Ken Rockot wrote:
> What about:
> 
> for (int reason = 1; reason < ...; reason <<= 1) {
> 
> instead?

Done.

https://codereview.chromium.org/412003003/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:145: if ((reason == 0 && reasons
== 0) || reasons & reason)
On 2014/07/24 03:56:02, Ken Rockot wrote:
> OK now I'm confused; I don't think reason can ever be 0 here.
> 
> Here's my guess for what the whole function should be, if I understand the
> intent correctly:
> 
> void RecordDisableReasons(int reasons) {
>   if (reasons == Extension::DISABLE_NONE) {
>     UMA_HISTOGRAM_ENUMERATION("Extensions.DisableReason",
> Extension::DISABLE_NONE, Extension::DISABLE_REASON_LAST);
>     return;
>   }
>   for (int reason = 1; reason < Extension::DISABLE_REASON_LAST; reason <<= 1)
{
>     if (reasons & reason) {
>       UMA_HISTOGRAM_ENUMERATION("Extensions.DisableReason", reason,
> Extension::DISABLE_REASON_LAST);
>     }
>   }
> }

And this is why we have code review! I did some refactoring and hadn't re-tested
the 0 case. Thanks - fixed.

[Ken Rockot(use gerrit already), 2014-07-24 17:23:57]

LGTM!

[asargent_no_longer_on_chrome, 2014-07-24 21:15:09]

On 2014/07/24 17:23:57, Ken Rockot wrote:
> LGTM!

isherman/jwd - ping

[Ilya Sherman, 2014-07-24 21:55:51]

https://codereview.chromium.org/412003003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/412003003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_system_impl.cc:241:
UMA_HISTOGRAM_BOOLEAN("Extensions.CorruptExtensionWouldBeDisabled", true);
It looks like you're probably planning to compare the values emitted to these
two histograms.  If so, it's probably a good idea to combine them into a single
histogram -- that tends to work better with the dashboard.

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

https://codereview.chromium.org/412003003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:145:
Extension::DISABLE_REASON_LAST);
Please define a small wrapper function around this UMA_HISTOGRAM_ENUMERATION
call, so that there's less chance that one of the code paths would have a typo
in the histogram name, for example.

https://codereview.chromium.org/412003003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:151: "Extensions.DisableReason",
reason, Extension::DISABLE_REASON_LAST);
UMA_HISTOGRAM_ENUMERATION allocates a vector to back the histogram.  Since these
values are sparse, please either use a sparse histogram
(UMA_HISTOGRAM_SPARSE_SLOWLY) or remap the values to a dense mapping for the
purpose of metrics.

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

https://codereview.chromium.org/412003003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:6624: +</histogram>
It would probably be useful for this histogram to have a built-in baseline of
how many times extensions were loaded in total (or, checked for corruption, if
that's not done at load time).  That is, assuming the main question you want to
answer is something like "What fraction of all extensions are corrupted?"

https://codereview.chromium.org/412003003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:6631: +    expected content hash.
So this is the total value over the lifetime of the client?  Are you concerned
at all that this has a bias that depends on the age of the client?

https://codereview.chromium.org/412003003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:37974: +  <int value="0" label="NONE"/>
Does "NONE" mean not disabled, or disabled for an unknown reason?

[asargent_no_longer_on_chrome, 2014-07-25 00:09:16]

Thanks for review! Responded to comments - please take another look.

https://codereview.chromium.org/412003003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/412003003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_system_impl.cc:241:
UMA_HISTOGRAM_BOOLEAN("Extensions.CorruptExtensionWouldBeDisabled", true);
On 2014/07/24 21:55:51, Ilya Sherman wrote:
> It looks like you're probably planning to compare the values emitted to these
> two histograms.  If so, it's probably a good idea to combine them into a
single
> histogram -- that tends to work better with the dashboard.

Actually probably not - one is emitted when the feature is in a bootstrapping
mode, and will help understand at what rate we expect extensions to be disabled
once we turn enforcement on, and the other records what actually happens when
enforcement is on.

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

https://codereview.chromium.org/412003003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:145:
Extension::DISABLE_REASON_LAST);
On 2014/07/24 21:55:51, Ilya Sherman wrote:
> Please define a small wrapper function around this UMA_HISTOGRAM_ENUMERATION
> call, so that there's less chance that one of the code paths would have a typo
> in the histogram name, for example.

Done.

https://codereview.chromium.org/412003003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:151: "Extensions.DisableReason",
reason, Extension::DISABLE_REASON_LAST);
On 2014/07/24 21:55:51, Ilya Sherman wrote:
> UMA_HISTOGRAM_ENUMERATION allocates a vector to back the histogram.  Since
these
> values are sparse, please either use a sparse histogram
> (UMA_HISTOGRAM_SPARSE_SLOWLY) or remap the values to a dense mapping for the
> purpose of metrics.

Done.

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

https://codereview.chromium.org/412003003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:6624: +</histogram>
On 2014/07/24 21:55:51, Ilya Sherman wrote:
> It would probably be useful for this histogram to have a built-in baseline of
> how many times extensions were loaded in total (or, checked for corruption, if
> that's not done at load time).  That is, assuming the main question you want
to
> answer is something like "What fraction of all extensions are corrupted?"

This is tricky - we already have a histogram for the number of extensions that
get loaded at startup, and in this CL I've also added one that buckets disabled
extensions by the disabled reason (corrupt contents being one of those reasons).
But this one is designed to help us see the rate at which extensions are getting
disabled (does the number per day stay roughly? does it ramp up to something
high when we turn on the feature and then drop to a small number after a few
days/weeks?)

https://codereview.chromium.org/412003003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:6631: +    expected content hash.
On 2014/07/24 21:55:51, Ilya Sherman wrote:
> So this is the total value over the lifetime of the client?  Are you concerned
> at all that this has a bias that depends on the age of the client?

That's an interesting thought - I don't think it will affect the kind of
analysis we want to do. This will help us answer questions like "what is the
total number of extensions that have become disabled since we turned the feature
on?".

https://codereview.chromium.org/412003003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:37974: +  <int value="0" label="NONE"/>
On 2014/07/24 21:55:51, Ilya Sherman wrote:
> Does "NONE" mean not disabled, or disabled for an unknown reason?

This corresponds to the DISABLE_NONE enum value in Extension::DisableReason, and
means "disabled but we don't know the specific reason" (possibly because we did
not always record reasons extensions were disabled at some point in the past).

[Ilya Sherman, 2014-07-25 03:14:53]

Histograms LGTM, thanks.

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

https://codereview.chromium.org/412003003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:37974: +  <int value="0" label="NONE"/>
On 2014/07/25 00:09:16, Antony Sargent wrote:
> On 2014/07/24 21:55:51, Ilya Sherman wrote:
> > Does "NONE" mean not disabled, or disabled for an unknown reason?
> 
> This corresponds to the DISABLE_NONE enum value in Extension::DisableReason,
and
> means "disabled but we don't know the specific reason" (possibly because we
did
> not always record reasons extensions were disabled at some point in the past).

Ok.  This is one of the rare cases where I think some tooltip text might help,
and would therefore suggest writing something like

<int value="1" label="NONE">Disabled for an unknown reason</int>

Or, even better, you could just have the label be "unknown" rather than "none"
-- though then the label doesn't match the enum value.

[asargent_no_longer_on_chrome, 2014-07-25 18:01:12]

On 2014/07/25 03:14:53, Ilya Sherman wrote:
> Histograms LGTM, thanks.
> 
>
https://codereview.chromium.org/412003003/diff/20001/tools/metrics/histograms...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/412003003/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:37974: +  <int value="0"
label="NONE"/>
> On 2014/07/25 00:09:16, Antony Sargent wrote:
> > On 2014/07/24 21:55:51, Ilya Sherman wrote:
> > > Does "NONE" mean not disabled, or disabled for an unknown reason?
> > 
> > This corresponds to the DISABLE_NONE enum value in Extension::DisableReason,
> and
> > means "disabled but we don't know the specific reason" (possibly because we
> did
> > not always record reasons extensions were disabled at some point in the
past).
> 
> Ok.  This is one of the rare cases where I think some tooltip text might help,
> and would therefore suggest writing something like
> 
> <int value="1" label="NONE">Disabled for an unknown reason</int>
> 
> Or, even better, you could just have the label be "unknown" rather than "none"
> -- though then the label doesn't match the enum value.

Done - switched NONE to UNKNOWN.

[asargent_no_longer_on_chrome, 2014-07-25 18:01:23]

The CQ bit was checked by asargent@chromium.org

[commit-bot: I haz the power, 2014-07-25 18:02:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asargent@chromium.org/412003003/80001

[commit-bot: I haz the power, 2014-07-25 21:45:29]

Change committed as 285688