Closed Bug 1433334 Opened 6 years ago Closed 6 years ago

Store the installation source of the extension for telemetry

Categories

(Toolkit :: Add-ons Manager, enhancement, P1)

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla64
Iteration:
64.1 - Sep 14
Tracking Flags:
Tracking Status
firefox64 --- fixed

People

(Reporter: andy+bugzilla, Assigned: rpl)

References

Details

Attachments

(1 file, 9 obsolete files)

Bug 1433334 - Store the install source of the extensions for telemetry. r=aswan!
46 bytes, text/x-phabricator-request
aswan
: review+
Details | Review 
The telemetry plans in bug 1316074 require us to store the source of the extension install. For example: when a disable event comes later, we'll be able to correlate that to the source of the install.

Note: this wouldn't store any URLs or file locations. The list of sources would be something like: "AMO", "Third Party", "Local", "Distribution", "Sync", "Sideloaded", "Temporary".

Note: add-ons installed before this patch lands would have a value of null, which is unfortunate.
Priority: -- → P3
(In reply to Andy McKay [:andym] from comment #0)
> Note: add-ons installed before this patch lands would have a value of null,
> which is unfortunate.

Bummer. By Firefox 62, we want to deprecate support for *unpacked* extensions that are sideloaded via the Windows registry. Can this probe be modified, or a new one added, that could tell us if an extension is packed or unpacked, keeping in mind support for loading temporary unpacked extensions will remain? This would obviously need to work for add-ons already installed, allowing us to identify those in Firefox 61 and use it for notification purposes.
Comment on attachment 8981474 [details]
Bug 1433334 - Store the install source of the extensions for telemetry.

As briefly discussed last week, I've started to explore in a bit more detail where we could store the information related to which was the "installation source" for an extension.

I've attached a patch here for an initial feedback about the approach (which also highlight some of the "source of installation" that we may be interested to recognize, in some cases we may just need to know it so that we can skip the telemetry events from something that we may not be interested in, e.g. we may not be interested to collect telemetry from permanent addons installed from marionette).

We need to keep track of the installation source starting from when an "installation flow" starts, which means that may not have a representation of the extension yet, and so the best place to store that information (at least from my current point of view based on what I saw) seems to be the AddonInstall base class defined in XPIDatabase.jsm (https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/toolkit/mozapps/extensions/internal/XPIInstall.jsm#1293), then that information is copied into the AddonInternal object (https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/toolkit/mozapps/extensions/internal/XPIDatabase.jsm#274) and persisted across the extension restarts (and also copied back into the AddonInstall object related to an extension update).

Having this information on both the AddonInstall and the AddonInternal objects also allow to detect when "the source of installation" is changing (e.g. when a user explicitly install an extension over one that was already installed, from a different installation source, e.g. the user may have installed an extension from a third party website and then install a more recent version manually from AMO).

I tried to keep most of the code that set the "install.initiationSource" in the AddonManager code itself, but while I've been tracking down the possible "sources of installation" I noticed that it may not be always possible yet, at least it is not if we don't change the signature of the AddonManager's methods that they have been calling to create a new installation), but in the meantime I opted to include all of them in the patch so that we may double-check if I've missed any (also the initiationSource is currently set using a plain string, but it would be more reasonable to define some consts and use them where needed).

(as a side note, 'initiationSource' was the name used in the telemetry docs, but I would be more than happy to use a different name that we may agreed on ;-))
Attachment #8981474 - Flags: feedback?(aswan)
Comment on attachment 8981474 [details]
Bug 1433334 - Store the install source of the extensions for telemetry.

https://reviewboard.mozilla.org/r/247576/#review254200

This looks reasonable.  Things might be simpler if we just add a source parameter to the AddonManager methods that create install objects rather than having callers set that property after creating an install object.

::: testing/marionette/addon.js:25
(Diff revision 1)
>    [-5]: "ERROR_SIGNEDSTATE_REQUIRED: The addon must be signed and isn't.",
>  };
>  
>  async function installAddon(file) {
>    let install = await AddonManager.getInstallForFile(file);
> +  install.initiationSource = "marionette";

Are we really collecting this?  I would expect telemetry to be disabled in any marionette-controlled profile.

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm:545
(Diff revision 1)
> +  get initiationSource() {
> +    if (LightweightThemeManager.isBuiltIn(themeFor(this))) {
> +      return "builtin-theme";
> +    }
> +
> +    return "lightweight-theme";

LWTs should be going away around 62, its probably okay to just skip them
Attachment #8981474 - Flags: feedback?(aswan) → feedback+
Iteration: --- → 63.4 - Aug 20
Attachment #8994305 - Flags: review?(aswan)
Comment on attachment 8981474 [details]
Bug 1433334 - Store the install source of the extensions for telemetry.

Hi Andrew,
the updated version of this patch adds the additional initiationSource parameter to the AddonManager methods that creates the AddonInstall instances as be agreed in the initial round of feedback.

The rest of the patch ensures that the code that lives in "toolkit/mozapps/extensions" dir and implements an "installation entry point" (e.g. the about:addons page, the handlers for the installTrigger and drag and drop of file and web urls, the Addon Manager WebAPI et.c) also provide the "initiationSource" parameter.

In this patch there are also defined two new helpers AddonManager.getInitiationSourceForPrincipal and AddonManager.getInitiationSourceForHost, which are tested in an xpcshell test unit. 

(There are also some additional asseriotions added to the existing AddonManager tests, which are meant to verify the initiationSource expected on the install and addon objects in a separate patch, attachment 8994305 [details], and I'm looking into adding more asap).
Attachment #8981474 - Flags: review?(aswan)
Comment on attachment 8981474 [details]
Bug 1433334 - Store the install source of the extensions for telemetry.

https://reviewboard.mozilla.org/r/247576/#review266546

::: toolkit/mozapps/extensions/AddonManager.jsm:2723
(Diff revision 2)
>        } catch (err) {
>          return Promise.reject({message: err.message});
>        }
>  
> -      return AddonManagerInternal.getInstallForURL(options.url, "application/x-xpinstall", options.hash)
> +      let initiationHost = AddonManager.getInitiationSourceForPrincipal(options.documentPrincipal);
> +      let initiationSource = `${initiationHost}|amWebAPI`;

Why do we need the |amWebAPI suffix here?

::: toolkit/mozapps/extensions/addonManager.js:93
(Diff revision 2)
> +    if (!AddonManager.isInstallAllowed(mimetype, triggeringPrincipal)) {
>        aCallback = null;
>        retval = false;
>      }
>  
> -    AddonManager.getInstallForURL(aUri, aMimetype, aHash, aName, aIcon, null, aBrowser).then(aInstall => {
> +    let initiationSource = AddonManager.getInitiationSourceForPrincipal(triggeringPrincipal);

Why use the principal here instead of just having callers of this function pass an appropriate source?  That would avoid having to write code for cases that shouldn't happen (eg, if we get a null principal here or something)

::: toolkit/mozapps/extensions/addonManager.js:95
(Diff revision 2)
> +    if (aPayload.fromInstallTrigger) {
> +      initiationSource += "|installTrigger";
> +    } else if (aPayload.fromContentHandler) {
> +      initiationSource += "|url";
> +    }

If we care about both the method and the host, how about if we just keep two separate properties?

::: toolkit/mozapps/extensions/internal/XPIDatabase.jsm:98
(Diff revision 2)
>  // Properties to cache and reload when an addon installation is pending
>  const PENDING_INSTALL_METADATA =
>      ["syncGUID", "targetApplications", "userDisabled", "softDisabled",
>       "existingAddonID", "sourceURI", "releaseNotesURI", "installDate",
> -     "updateDate", "applyBackgroundUpdates", "compatibilityOverrides"];
> +     "updateDate", "applyBackgroundUpdates", "compatibilityOverrides",
> +     "initiationSource"];

I know this is all over the place and changing it will be a hassle, but can we call this "installationSource" instead?
Attachment #8981474 - Flags: review?(aswan)
Comment on attachment 8981474 [details]
Bug 1433334 - Store the install source of the extensions for telemetry.

https://reviewboard.mozilla.org/r/247576/#review254200

> Are we really collecting this?  I would expect telemetry to be disabled in any marionette-controlled profile.

I would also expect them to not be collected because the telemetry is actually disabled, nevertheless marionette is also used internally when a Firefox instance is being controlled by webdriver and so I preferred to mark these installations with marionette as their source, so that we can filter them out from the events if they have been collected (e.g. because webdriver has been used on a profile which has the telemetry enabled).

> LWTs should be going away around 62, its probably okay to just skip them

Exactly, this is basically the reason why I've not even tried to put there their real source of installation, they are just marked with source "lightweigth-theme" (just in case they are already there, otherwise no events is supposed to be collected with such source).
Comment on attachment 8981474 [details]
Bug 1433334 - Store the install source of the extensions for telemetry.

https://reviewboard.mozilla.org/r/247576/#review266546

> Why do we need the |amWebAPI suffix here?

While looking into the telemetry events that I was collecting when installing from the disco panel, AMO and testpilot, I noticed that we may want to be able to check if the source is installing the addons using the installTrigger or the amWebAPI (and, as an example, AMO is currently using the installTrigger but should migrate soon to the amWebAPI), or if the addon has been installed by navigating a window to an xpi url.

To be honest, I was tempted to add a separate property in the telemetry event (e.g. an additional extra var named "method"), and in the updated patch I've opted for this strategy (as you were also suggesting in one of the other review comments).

> Why use the principal here instead of just having callers of this function pass an appropriate source?  That would avoid having to write code for cases that shouldn't happen (eg, if we get a null principal here or something)

I would really prefer to share the same logic that converts a principal into the source string included in the telemetry events in both this scenario and the one covered by AddonManager.createInstall (where this helper method is also used to convert the document principal into an installation source).

This helper is tested with an xpcshell unit test (in the test file named test_getInstallSourceFromPrincipal.js) and so the cases for an undefined principal or any other kind of principals are only tested by that unit test.

> If we care about both the method and the host, how about if we just keep two separate properties?

+1! I was also tempted to do so, and so I opted for two separate properties (source and method, method is only included when a source may use more than one method).

> I know this is all over the place and changing it will be a hassle, but can we call this "installationSource" instead?

hehe, no worries, I couldn't agree more (e.g. in the end of comment 3, I wrote: "as a side note, 'initiationSource' was the name used in the telemetry docs, but I would be more than happy to use a different name that we may agreed on ;-)")

I opted to rename `initiationSource` into `installTelemetryInfo` everywhere in the patches, because:

- it makes immediately clear why we are collecting these properties (to include them into the addon manager telemetry)
- `installTelemetryInfo` is now an object which may have two properties: `source` and `method`
- as a property of the addon, it makes clear that these are information related to the addon install.
Comment on attachment 8981474 [details]
Bug 1433334 - Store the install source of the extensions for telemetry.

https://reviewboard.mozilla.org/r/247576/#review254200

> I would also expect them to not be collected because the telemetry is actually disabled, nevertheless marionette is also used internally when a Firefox instance is being controlled by webdriver and so I preferred to mark these installations with marionette as their source, so that we can filter them out from the events if they have been collected (e.g. because webdriver has been used on a profile which has the telemetry enabled).

geckodriver and Marionette instruments the profile to not collect
Telemetry data by default, but these preferences can indeed be
overridden by a user.
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Comment on attachment 8981474 [details]
Bug 1433334 - Store the install source of the extensions for telemetry.

https://reviewboard.mozilla.org/r/247576/#review269604

::: toolkit/mozapps/extensions/AddonManager.jsm:1589
(Diff revision 4)
>     *         Optional placeholder icons while the add-on is being downloaded
>     * @param  aVersion
>     *         An optional placeholder version while the add-on is being downloaded
>     * @param  aBrowser
>     *         An optional <browser> element for download permissions prompts.
> +   * @param  aInstallTelemetryInfo

nit: since this whole method is about installs, this could be just named `aTelemetryInfo` to keep everything more concise.

::: toolkit/mozapps/extensions/addonManager.js:97
(Diff revision 4)
>  
> -    AddonManager.getInstallForURL(aUri, aMimetype, aHash, aName, aIcon, null, aBrowser).then(aInstall => {
> -      function callCallback(uri, status) {
> +    let installTelemetryInfo = {
> +      source: AddonManager.getInstallSourceFromPrincipal(triggeringPrincipal),
> +    };
> +
> +    if (aPayload.fromInstallTrigger) {

How about just adding a `installMethod` property here and have InstallTrigger set it to "installTrigger", content handler set it to "url", etc. then we can just copy the property here rather than doing this translation.

::: toolkit/mozapps/extensions/amWebAPI.js:234
(Diff revision 4)
> +      // Provide the documentPrincipal from which the amWebAPI is being called
> +      // (so that we can detect if the API is being used from the disco pane,
> +      // AMO, testpilot or another unknown webpage).
> +      documentPrincipal: this.window.document.nodePrincipal,

echo of a previous comment, but how about just grabbing location.href instead?
Attachment #8981474 - Flags: review?(aswan)
Comment on attachment 8981474 [details]
Bug 1433334 - Store the install source of the extensions for telemetry.

https://reviewboard.mozilla.org/r/247576/#review266546

> I would really prefer to share the same logic that converts a principal into the source string included in the telemetry events in both this scenario and the one covered by AddonManager.createInstall (where this helper method is also used to convert the document principal into an installation source).
> 
> This helper is tested with an xpcshell unit test (in the test file named test_getInstallSourceFromPrincipal.js) and so the cases for an undefined principal or any other kind of principals are only tested by that unit test.

I'm not sure why we need to extract it from a principal at all?  Its a little more plumbing, but why not have things like amInstallTrigger, amContentHandler, etc. just grab the current window.document.location.href and include that in the message that is sent to the parent process?
Blocks: 1486761
Blocks: 1486762
Blocks: 1486763
Blocks: 1486764
Blocks: 1486765
Blocks: 1486766
Comment on attachment 8994306 [details]
Bug 1433334 - Add installTelemetryInfo to addon installed from EnterprisePolicies.

Marking as obsolete the patch part of the mozreview, I'm going to move it to the follow up Bug 1486761.
Attachment #8994306 - Attachment is obsolete: true
Comment on attachment 8994307 [details]
Bug 1433334 - Add installTelemetryInfo to addon installed from WebIDE.

Marking as obsolete the patch part of the mozreview, I'm going to move it to the follow up Bug 1486762.
Attachment #8994307 - Attachment is obsolete: true
Comment on attachment 8994308 [details]
Bug 1433334 - Add installTelemetryInfo to addon installed from Firefox Sync.

Marking as obsolete the patch part of the mozreview, I'm going to move it to the follow up Bug 1486763.
Attachment #8994308 - Attachment is obsolete: true
Comment on attachment 8994309 [details]
Bug 1433334 - Add installTelemetryInfo to addon installed from AddonStudies.

Marking as obsolete the patch part of the mozreview, I'm going to move it to the follow up Bug 1486764.
Attachment #8994309 - Attachment is obsolete: true
Comment on attachment 8994310 [details]
Bug 1433334 - Add installTelemetryInfo to addon installed from Marionette.

Marking as obsolete the patch part of the mozreview, I'm going to move it to the follow up Bug 1486765.
Attachment #8994310 - Attachment is obsolete: true
Comment on attachment 8994312 [details]
Bug 1433334 - Add installTelemetryInfo to distro addons installed on Firefox for Android.

Marking as obsolete the patch part of the mozreview, I'm going to move it to the follow up Bug 1486766.
Attachment #8994312 - Attachment is obsolete: true
- Implemented stored "install source of the extensions" for telemetry events.
- Added test assertions related to the expected install and addon installTelemetryInfo.
- Added installTelemetryInfo to addon installed from "drag-and-drop" file urls on Firefox Desktop.
Comment on attachment 8981474 [details]
Bug 1433334 - Store the install source of the extensions for telemetry.

Marking the patches part of the mozreview request as obsolete, in favor of the new patch submitted on Phabricator (attachment 9004551 [details]).
Attachment #8981474 - Attachment is obsolete: true
Comment on attachment 8994305 [details]
Bug 1433334 - Add test assertions related to the expected install and addon installTelemetryInfo.

Marking the patches part of the mozreview request as obsolete, in favor of the new patch submitted on Phabricator (attachment 9004551 [details]).
Attachment #8994305 - Attachment is obsolete: true
Comment on attachment 8994311 [details]
Bug 1433334 - Add installTelemetryInfo to addon installed from "drag-and-drop" file urls on Firefox Desktop.

Marking the patches part of the mozreview request as obsolete, in favor of the new patch submitted on Phabricator (attachment 9004551 [details]).
Attachment #8994311 - Attachment is obsolete: true
Priority: P3 → P1
Blocks: 1433335
Comment on attachment 9004551 [details]
Bug 1433334 - Store the install source of the extensions for telemetry. r=aswan!

Andrew Swan [:aswan] has approved the revision.
Attachment #9004551 - Flags: review+
Iteration: 63.4 - Aug 20 → 64.1 (Sep 14)
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/9c6f847c163b
Store the install source of the extensions for telemetry. r=aswan
https://hg.mozilla.org/mozilla-central/rev/9c6f847c163b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
I'm setting qe-verify- because this is tested in automation (and it may be a bit tricky to manually verify the telemetryInfo stored internally but not yet exposed anywhere), and it is also going to be "QA verified" as part of Bug 1433335 (using the STRs from Bug 1433335 comment 44).
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: