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

chrome/browser/extensions/installed_loader.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/installed_loader.h"
 
 #include "base/files/file_path.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 205 matching lines @@
     ExtensionInfo* info = extensions_info->at(i).get();
 
     // Skip extensions that were loaded from the command-line because we don't
     // want those to persist across browser restart.
     if (info->extension_location == Manifest::COMMAND_LINE)
       continue;
 
     ManifestReloadReason reload_reason = ShouldReloadExtensionManifest(*info);
     ++reload_reason_counts[reload_reason];
     UMA_HISTOGRAM_ENUMERATION("Extensions.ManifestReloadEnumValue",
                               reload_reason, 100);

[jar (doing other things), 2014/05/20 17:43:34]

This should use a precise top value + 1, rather than 100.

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

Done.

 
     if (reload_reason != NOT_NEEDED) {
       // Reloading an extension reads files from disk.  We do this on the
       // UI thread because reloads should be very rare, and the complexity
       // added by delaying the time when the extensions service knows about
       // all extensions is significant.  See crbug.com/37548 for details.
       // |allow_io| disables tests that file operations run on the file
       // thread.
       base::ThreadRestrictions::ScopedAllowIO allow_io;
 
@@ skipping 72 matching lines @@
        ++iter) {
     const Extension* extension = *iter;
     Manifest::Location location = extension->location();
     Manifest::Type type = extension->GetType();
 
     // For the first few metrics, include all extensions and apps (component,
     // unpacked, etc). It's good to know these locations, and it doesn't
     // muck up any of the stats. Later, though, we want to omit component and
     // unpacked, as they are less interesting.
     if (extension->is_app())
       UMA_HISTOGRAM_ENUMERATION("Extensions.AppLocation", location, 100);

[jar (doing other things), 2014/05/20 17:43:34]

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)

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

Done.

     else if (extension->is_extension())
       UMA_HISTOGRAM_ENUMERATION("Extensions.ExtensionLocation", location, 100);
 
     if (!ManifestURL::UpdatesFromGallery(extension)) {
       UMA_HISTOGRAM_ENUMERATION(
           "Extensions.NonWebstoreLocation", location, 100);
 
       // Check for inconsistencies if the extension was supposedly installed
       // from the webstore.
       enum {
         BAD_UPDATE_URL = 0,
         // This value was a mistake. Turns out sideloaded extensions can
         // have the from_webstore bit if they update from the webstore.
         DEPRECATED_IS_EXTERNAL = 1,
       };
       if (extension->from_webstore()) {
         UMA_HISTOGRAM_ENUMERATION(
             "Extensions.FromWebstoreInconsistency", BAD_UPDATE_URL, 2);

[jar (doing other things), 2014/05/20 17:43:34]

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.

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

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

       }
     }
 
     if (Manifest::IsExternalLocation(location)) {
       // See loop below for DISABLED.
       if (ManifestURL::UpdatesFromGallery(extension)) {
         UMA_HISTOGRAM_ENUMERATION("Extensions.ExternalItemState",
                                   EXTERNAL_ITEM_WEBSTORE_ENABLED,
                                   EXTERNAL_ITEM_MAX_ITEMS);
       } else {
@@ skipping 15 matching lines @@
         URLOverrides::GetChromeURLOverrides(extension).count("newtab")) {
       ++non_webstore_ntp_override_count;
     }
 
     // Don't count unpacked extensions anymore, either.
     if (Manifest::IsUnpackedLocation(location))
       continue;
 
     UMA_HISTOGRAM_ENUMERATION("Extensions.ManifestVersion",
                               extension->manifest_version(),
                               10);

[jar (doing other things), 2014/05/20 17:43:34]

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.

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

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?

[jar (doing other things), 2014/05/22 02:19:07]

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.  

 
     // We might have wanted to count legacy packaged apps here, too, since they
     // are effectively extensions. Unfortunately, it's too late, as we don't
     // want to mess up the existing stats.
     if (type == Manifest::TYPE_EXTENSION) {
       UMA_HISTOGRAM_ENUMERATION("Extensions.BackgroundPageType",
                                 GetBackgroundPageType(extension),
                                 10);
     }
 
     // Using an enumeration shows us the total installed ratio across all users.
     // Using the totals per user at each startup tells us the distribution of
     // usage for each user (e.g. 40% of users have at least one app installed).
     UMA_HISTOGRAM_ENUMERATION("Extensions.LoadType", type, 100);

[jar (doing other things), 2014/05/20 17:43:34]

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.

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

Done.

     switch (type) {
       case Manifest::TYPE_THEME:
         ++theme_count;
         break;
       case Manifest::TYPE_USER_SCRIPT:
         ++user_script_count;
         break;
       case Manifest::TYPE_HOSTED_APP:
         ++hosted_app_count;
         if (Manifest::IsExternalLocation(location)) {
@@ skipping 38 matching lines @@
 
     if (ManagedModeInfo::IsContentPack(extension))
       ++content_pack_count;
 
     RecordCreationFlags(extension);
 
     ExtensionService::RecordPermissionMessagesHistogram(
         extension, "Extensions.Permissions_Load");
 
     // For incognito and file access, skip anything that doesn't appear in
-    // settings.
-    if (extension->ShouldDisplayInExtensionSettings()) {
+    // settings. Also, policy-installed (and unpacked of course, checked above)
+    // extensions are boring.
+    if (extension->ShouldDisplayInExtensionSettings() &&
+        !Manifest::IsPolicyLocation(extension->location())) {
       if (extension->can_be_incognito_enabled()) {
         if (util::IsIncognitoEnabled(extension->id(), profile))
           ++incognito;
         else
           ++not_incognito;
       }
       if (extension->wants_file_access()) {
         if (util::AllowFileAccess(extension->id(), profile))
           ++file_access;
         else
@@ skipping 62 matching lines @@
   UMA_HISTOGRAM_COUNTS_100("Extensions.LoadUserScript", user_script_count);
   UMA_HISTOGRAM_COUNTS_100("Extensions.LoadTheme", theme_count);
   UMA_HISTOGRAM_COUNTS_100("Extensions.LoadPageAction", page_action_count);
   UMA_HISTOGRAM_COUNTS_100("Extensions.LoadBrowserAction",
                            browser_action_count);
   UMA_HISTOGRAM_COUNTS_100("Extensions.LoadContentPack", content_pack_count);
   UMA_HISTOGRAM_COUNTS_100("Extensions.DisabledForPermissions",
                            disabled_for_permissions_count);
   UMA_HISTOGRAM_COUNTS_100("Extensions.NonWebStoreNewTabPageOverrides",
                            non_webstore_ntp_override_count);
-  UMA_HISTOGRAM_COUNTS_100("Extensions.IncognitoAllowed", incognito);
-  UMA_HISTOGRAM_COUNTS_100("Extensions.IncognitoNotAllowed", not_incognito);
-  UMA_HISTOGRAM_COUNTS_100("Extensions.FileAccessAllowed", file_access);
-  UMA_HISTOGRAM_COUNTS_100("Extensions.FileAccessNotAllowed", not_file_access);
+  if (incognito + not_incognito > 0) {

[jar (doing other things), 2014/05/20 17:43:34]

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:10]

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

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

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.

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

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]

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

+    UMA_HISTOGRAM_COUNTS_100("Extensions.IncognitoAllowed", incognito);
+    UMA_HISTOGRAM_COUNTS_100("Extensions.IncognitoNotAllowed", not_incognito);
+  }
+  if (file_access + not_file_access > 0) {
+    UMA_HISTOGRAM_COUNTS_100("Extensions.FileAccessAllowed", file_access);
+    UMA_HISTOGRAM_COUNTS_100("Extensions.FileAccessNotAllowed",
+                             not_file_access);
+  }
 }
 
 int InstalledLoader::GetCreationFlags(const ExtensionInfo* info) {
   int flags = extension_prefs_->GetCreationFlags(info->extension_id);
   if (!Manifest::IsUnpackedLocation(info->extension_location))
     flags |= Extension::REQUIRE_KEY;
   if (extension_prefs_->AllowFileAccess(info->extension_id))
     flags |= Extension::ALLOW_FILE_ACCESS;
   return flags;
 }
 
 }  // namespace extensions