New issue
Advanced search Search tips

Issue 661405 link

Starred by 1 user
Status: Fixed
Owner:
wychen@chromium.org
Closed: Nov 2016
Cc:
cylee@chromium.org
benwells@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 661401



Sign in to add a comment

Extensions.AppLaunchSource off-by-one error

Project Member Reported by wychen@chromium.org, Nov 2 2016 
In this code snippet, the type passed to UMA_HISTOGRAM_ENUMERATION doesn't match histograms.xml.

In https://cs.chromium.org/chromium/src/extensions/browser/api/app_runtime/app_runtime_api.cc?dr=CSs&sq=package:chromium&rcl=1478013686&l=55 :

void DispatchOnLaunchedEventImpl(
    const std::string& extension_id,
    app_runtime::LaunchSource source,
    ...

  UMA_HISTOGRAM_ENUMERATION(
      "Extensions.AppLaunchSource", source, NUM_APP_LAUNCH_SOURCES);


app_runtime::LaunchSource is defined as:

enum LaunchSource {
  LAUNCH_SOURCE_NONE,
  LAUNCH_SOURCE_UNTRACKED,
  LAUNCH_SOURCE_APP_LAUNCHER,
  ...
  LAUNCH_SOURCE_TEST,
  LAUNCH_SOURCE_INSTALLED_NOTIFICATION,
  LAUNCH_SOURCE_LAST = LAUNCH_SOURCE_INSTALLED_NOTIFICATION,
};


However, in histograms.xml:

<enum name="AppLaunchSource" type="int">
  <int value="0" label="SOURCE_UNTRACKED"/>
  <int value="1" label="SOURCE_APP_LAUNCHER"/>
  ...
  <int value="18" label="SOURCE_TEST"/>
  <int value="19" label="SOURCE_INSTALLED_NOTIFICATION"/>
</enum>


This mismatch has caused off-by-one error on UMA data analysis.

This is discovered by the checks implemented in this CL:
https://codereview.chromium.org/2469993002/

Comment 1 by wychen@chromium.org, Nov 2 2016

Blocking: 661401

Comment 2 by wychen@chromium.org, Nov 2 2016

Cc: cylee@chromium.org benwells@chromium.org

Comment 3 by wychen@chromium.org, Nov 2 2016 

I think the best way to fix this is to edit histograms.xml and change the labels to match LaunchSource, and use LAUNCH_SOURCE_LAST+1 as the max.

Comment 4 by benwells@chromium.org, Nov 2 2016 

How weird. There is also https://cs.chromium.org/chromium/src/extensions/common/constants.h?rcl=1478101093&l=122:

enum AppLaunchSource {
  SOURCE_UNTRACKED = 0,
  SOURCE_APP_LAUNCHER,
  SOURCE_NEW_TAB_PAGE,
  SOURCE_RELOAD,
  SOURCE_RESTART,
  SOURCE_LOAD_AND_LAUNCH,
  SOURCE_COMMAND_LINE,
  SOURCE_FILE_HANDLER,
  SOURCE_URL_HANDLER,
  SOURCE_SYSTEM_TRAY,
  SOURCE_ABOUT_PAGE,
  SOURCE_KEYBOARD,
  SOURCE_EXTENSIONS_PAGE,
  SOURCE_MANAGEMENT_API,
  SOURCE_EPHEMERAL_APP_DEPRECATED,
  SOURCE_BACKGROUND,
  SOURCE_KIOSK,
  SOURCE_CHROME_INTERNAL,
  SOURCE_TEST,
  SOURCE_INSTALLED_NOTIFICATION,

  NUM_APP_LAUNCH_SOURCES
};

I don't really know why we have two enumerations. cylee@, can you remember?

Comment 5 by wychen@chromium.org, Nov 2 2016 

I guess the original intention was to use AppLaunchSource for UMA, but LaunchSource was wrongly passed to UMA_HISTOGRAM_ENUMERATION.

For data consistency, we can no longer "fix" it by using the original intention.

As for why having two enum, could it be that one is generated?

Comment 6 by benwells@chromium.org, Nov 2 2016 

Right, one was auto generated from the IDL, but I guess was a bit too high in the dependency stack to use everywhere, so another was defined for general use.

Importantly, the values passed into the histogram will in fact always be of the autogenerated type. It would have been better to pass the other type in but it's too late to change things.

So I think what we should do is:
- update the histograms.xml as suggested to match LaunchSource.
- use LAUNCH_SOURCE_LAST instead of NUM_APP_LAUNCH_SOURCES as the last launch source
- for clarity add the _NONE value to the constants.
- add a comment to the .idl file mentioning that these should not be reordered etc. as they are used in histograms.

Comment 7 by wychen@chromium.org, Nov 2 2016

Status: Available (was: Untriaged)
Hopefully no important decisions were made based on the distribution of "Extensions.AppLaunchSource" in UMA.

Comment 9 by wychen@chromium.org, Nov 17 2016

Owner: wychen@chromium.org
Status: Fixed (was: Available)
►

Sign in to add a comment