1435253 - [Normandy Telemetry] Corrupt add-on ("install-failure") doesn't log telemetry

Status: VERIFIED FIXED

(Firefox :: Normandy Client, defect, P1)

Description

[Adrian Florinescu [:aflorinescu]]

[Environments:]
Ubuntu 16.04 x64
Firefox Nightly 60.0a1 20180201220225


[Prerequisites]
You need access to Admin interface (https://normandy-admin.stage.mozaws.net/control) or New Admin interface (https://normandy-admin.stage.mozaws.net/control-new)
1. Obtain a copy of Firefox with the SHIELD recipe client system add-on installed. You can check about:support to ensure that you have it.
2. Set the extensions.shield-recipe-client.dev_mode preference to true to run recipes immediately on startup.
3. Set the extensions.shield-recipe-client.logging.level preference to 0 to enable more logging.
4. Set the security.content.signature.root_hash preference to DB:74:CE:58:E4:F9:D0:9E:E0:42:36:BE:6C:C5:C4:F6:6A:E7:74:7D:C0:21:42:7A:03:BC:2F:57:0C:8B:9B:90.
5. Set the preference value for extensions.shield-recipe-client.api_url set to https://normandy.stage.mozaws.net/api/v1.

[Steps:]
1. Open Admin Interface.
2. Click New Extension and upload a corrupted extension. (On staging extension/2/ is the corrupted one I used to test with) 
3. Create/Approve/Publish a new opt-out-study type recipe using the step 2 corrupted add-on as extension.
4. Create a new profile, set the pre-requisites, restart browser.
5. Open the Browser console.
6. Open about:telemetry /Current ping/Events


[Actual Result:]
5. In the browser console Normandy would throw an error that the add-on is corrupted and install failed.
6. Telemtry / Current ping / Events is not found.

[Expected Result:]
5. In the browser console Normandy would throw an error that the add-on is corrupted and install failed.
6. Telemtry / Current ping / Events / Dynamic telemetry event is found and states that the study is unenrolled - ("install-failure")

Comment 1

[Michael Cooper [:mythmon]]

Attachment: Bug 1435253 - Handle more types of failure during add-on study enrollment

Review commit: https://reviewboard.mozilla.org/r/218240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218240/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8948855 [details]
Bug 1435253 - Handle more types of failure during add-on study enrollment

https://reviewboard.mozilla.org/r/218240/#review224132

::: browser/extensions/shield-recipe-client/lib/AddonStudies.jsm:288
(Diff revision 1)
>        return study;
>      } catch (err) {
>        await getStore(db).delete(recipeId);
>  
> -      TelemetryEvents.sendEvent("unenroll", "addon_study", name, {
> -        reason: "install-failure",
> +      TelemetryEvents.sendEvent("enroll_failed", "addon_study", name, {
> +        reason: (err.message || "unknown").slice(0, 80),  // max length is 80 chars

Note that the error message could potentially include personal information, like file paths with embedded user names and so on (e.g. "Couldn't write file to <wherever>").

Do we think that's an acceptable risk?

Comment 3

[:Gijs (he/him)]

Comment on attachment 8948855 [details]
Bug 1435253 - Handle more types of failure during add-on study enrollment

https://reviewboard.mozilla.org/r/218240/#review224136

Err, it seems try failed:

 FAIL | browser/extensions/shield-recipe-client/test/browser/browser_EventEmitter.js | head.js import threw an exception

at resource://shield-recipe-client/lib/TelemetryEvents.jsm:16 - Error: Method names should match the identifier pattern.

Looks like that comes from:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryEvent.cpp#1035

Looks like underscores are not valid in method names.

Perhaps name it `enrollFailure` (keep in mind to update tests + documentation as well) ? Still surprised that this would have passed locally though...

Comment 4

[Michael Cooper [:mythmon]]

Comment on attachment 8948855 [details]
Bug 1435253 - Handle more types of failure during add-on study enrollment

https://reviewboard.mozilla.org/r/218240/#review224132

> Note that the error message could potentially include personal information, like file paths with embedded user names and so on (e.g. "Couldn't write file to <wherever>").
> 
> Do we think that's an acceptable risk?

I forget that path names can be PII. I'll change this have less info in it, so it should be non-PII.

Comment 5

[Michael Cooper [:mythmon]]

Comment on attachment 8948855 [details]
Bug 1435253 - Handle more types of failure during add-on study enrollment

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218240/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ecf23ddec1e
Handle more types of failure during add-on study enrollment r=Gijs

Comment 7

[Michael Cooper [:mythmon]]

Adrian: A note for verifying this, the expected behavior has changed. Instead of an enroll/unenroll pair, we now expect an enrollFailed event for this action.

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/2ecf23ddec1e

Comment 9

[Adrian Florinescu [:aflorinescu]]

Fix verified on Windows 8.1 x64, MacOsx 10.12 and Ubuntu 16.04x64 using the latest nightly: 60.0a1 2018-02-09.

Example of logged telemetry with the fix:
"40783 normandy enrollFailed addon_study a {"reason": "resource://shield-recipe-client/lib/Addons.jsm:98:16 Error"}"

[Note:] We haven't validated yet the telemetry data on server, since it wasn't yet accessible, but will open a follow-up if there are any problems there.