ship Telemetry Coverage SAO update

RESOLVED FIXED

Status

enhancement
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: rhelmer, Assigned: rhelmer)

Tracking

Details

Attachments

(3 attachments, 15 obsolete attachments)

809 bytes, application/x-javascript
Details
2.52 KB, application/x-xpinstall
Details
6.67 KB, application/x-xpinstall
Details
Assignee

Description

10 months ago
Per https://blog.mozilla.org/data/2018/08/20/effectively-measuring-search-in-firefox/ we want to ship an extension (system add-on update aka SAO) which will activate for only 1% of users and send in a privacy-preserving bit of diagnostic info.
Assignee

Comment 1

10 months ago
Instructions for QA:

Overall this is a pretty typical system add-on update. It is a legacy extension, and only runs for 1% of users, but you can make yourself be in that 1% using the attached script (which can be run in the Browser Console).

I've already run this script and you can use this Telemetry client ID:
53fcb8b2-781a-3949-8b80-59bf15732200

This can be set in your profile (when Firefox is not running) in:
prefs.js ("toolkit.telemetry.cachedClientID" pref)
datareporting/state.json

If you start Nightly and use about:debugging, then you can load the unsigned extension (I am going to attach the extension momentarily and request signing+staging so it will work on Release)

I was able to confirm this was working via the "network" tab in the Browser Toolbox, if the extension is working there should be a connection to https://telemetry-coverage.mozilla.org with a payload similar to:

{
   "appVersion": "63.0a1",
   "appUpdateChannel": "nightly",
   "osName": "Darwin",
   "osVersion": "17.7.0",
   "telemetryEnabled": true
}

There should not be any UUID or other identifying info sent along with this payload, and note that it goes to a different endpoint than regular Telemetry.

The only other thing to test here is that this extension has a special boolean opt-out pref: "toolkit.telemetry.coverage.opt-out". This pref does not exist by default and must be created, if set to true then the extension should not send a payload as above for users in the 1% sample (such as the Telemetry client ID above will be)
Assignee

Comment 2

10 months ago
Please sign this system add-on update, thanks!
Flags: needinfo?(wezhou)
Flags: needinfo?(jthomas)
Assignee

Comment 3

10 months ago
(In reply to Robert Helmer [:rhelmer] from comment #2)
> Created attachment 9005525 [details]
> System add-on update for telemetry coverage (v1.0), unsigned
> 
> Please sign this system add-on update, thanks!

Rehan, I am out tomorrow - would you mind staging this on the test channel (for release) when the signed version is ready? Thanks!
Flags: needinfo?(rdalal)
I'm on PTO tomorrow and out till Tuesday. Tagging mythmon in.
Flags: needinfo?(rdalal)

Updated

10 months ago
Flags: needinfo?(jthomas)
Assignee

Updated

10 months ago
See Also: → 1480194

Comment 5

10 months ago
Posted file signed.9005525.xpi (obsolete) —
Signed file attached. Please test.
Flags: needinfo?(wezhou)
Sorry, I'm not confident doing this change without talking too Rehan or Rob. There is too much implied indirection for me to pick this up without documentation.

Re-tagging Rob, since he should be back before Rehan.
Flags: needinfo?(rhelmer)
This is now on the test channel (release-sysaddon) and pending sign off on release.

It has also replaced the previous version that was there (from bug 1480194). Let me know if they needed to be bundled.
Flags: needinfo?(rhelmer)
Assignee

Comment 8

10 months ago
(In reply to Rehan Dalal [:rehan, :rdalal] from comment #7)
> This is now on the test channel (release-sysaddon) and pending sign off on
> release.

Thanks!

> It has also replaced the previous version that was there (from bug 1480194).
> Let me know if they needed to be bundled.

Yes this should replace the previous version, thanks for the confirmation.

Comment 9

10 months ago
I tried using the singed extension attached (signed.9005525.xpi) and load it this time in about:addons by following the same steps mentioned before, but I received this message  "Telemetry coverage could not be installed because it's not compatible with firefox 62 " and I'm not sure what I'm doing wrong.

I also tried to trigger it using the following command "Components.utils.import("resource://gre/modules/AddonManager.jsm"); AddonManagerPrivate.backgroundUpdateCheck();" but nothing happened. (Please note that I updated the update channel to "release-sysaddon" value)

I am not sure what I am doing wrong.
(In reply to Hani Yacoub from comment #9)
> I tried using the singed extension attached (signed.9005525.xpi) and load it
> this time in about:addons by following the same steps mentioned before, but
> I received this message  "Telemetry coverage could not be installed because
> it's not compatible with firefox 62 " and I'm not sure what I'm doing wrong.
> 
> I also tried to trigger it using the following command
> "Components.utils.import("resource://gre/modules/AddonManager.jsm");
> AddonManagerPrivate.backgroundUpdateCheck();" but nothing happened. (Please
> note that I updated the update channel to "release-sysaddon" value)
> 
Right now as far as I can tell this is set up to be offered to 61.x, *not* 62.0.

On 62.0rc2, I set extensions.systemAddon.update.url to https://aus5.mozilla.org/update/3/SystemAddons/61.0/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/release-sysaddon/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml (replacing %VERSION% with 61.0 to fake it, and %CHANNEL% with release-sysaddon), then ran AddonManagerPrivate.backgroundUpdateTimerHandler() in the browser console, and the addon was installed.
(In reply to Julien Cristau [:jcristau] from comment #10)
> Right now as far as I can tell this is set up to be offered to 61.x, *not*
> 62.0.

BTW the addon manifest says <em:minVersion>61.*</em:minVersion>, I seem to remember that this means it will refuse to install on 61.0.x because "61.*" is greater than 61.0.x; should that be 61.0, or am I misremembering?

Comment 12

10 months ago
Posted image params.png (obsolete) —
Thank you Julien for the details. I followed the steps you mentioned above on Firefox 62.0rc2 and I set up the followings: 
- Took the Telemetry client ID: 53fcb8b2-781a-3949-8b80-59bf15732200
and set it the profile (when Firefox is not running) in: prefs.js ("toolkit.telemetry.cachedClientID" pref) and datareporting/state.json
- I set extensions.systemAddon.update.url to "https://aus5.mozilla.org/update/3/SystemAddons/61.0/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/release-sysaddon/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml" and triggered the system addons

The request URL displayed in the console looks a bit different that the one mentioned in comment 1. (please see headers.png and params.png screenshots attached)

And I'm not sure it this is the expected behavior.
Should it be installed on other Firefox version besides 62?
Flags: needinfo?(rhelmer)

Comment 13

10 months ago
Posted image headers.png (obsolete) —
(In reply to Hani Yacoub from comment #13)
> Created attachment 9006229 [details]
> headers.png

That shows a connection to versioncheck-bg.addons.mozilla.org, not telemetry-coverage.m.o?

Comment 15

10 months ago
The URL also contains "bug1487578@mozilla.org">telemetry-coverage-bug1487578@mozilla.org" that why I wasn't sure about the results.
Assignee

Comment 16

10 months ago
Did Julien's comments answer your concern or is there anything you wanted me to look at still? Thanks!
Flags: needinfo?(rhelmer) → needinfo?(hani.yacoub)
Assignee

Comment 17

10 months ago
(In reply to Julien Cristau [:jcristau] from comment #11)
> (In reply to Julien Cristau [:jcristau] from comment #10)
> > Right now as far as I can tell this is set up to be offered to 61.x, *not*
> > 62.0.
> 
> BTW the addon manifest says <em:minVersion>61.*</em:minVersion>, I seem to
> remember that this means it will refuse to install on 61.0.x because "61.*"
> is greater than 61.0.x; should that be 61.0, or am I misremembering?

Ack you are right, good catch (from the browser console on Release w/ extension logging on):

addons.xpi	WARN	System add-on bug1487578@mozilla.org">telemetry-coverage-bug1487578@mozilla.org isn't compatible with the application.

I'll update the XPI momentarily.
Assignee

Comment 18

10 months ago
Please sign this updated version of the Telemetry Coverage system add-on update. Thanks!
Flags: needinfo?(wezhou)
Flags: needinfo?(jthomas)
Flags: needinfo?(hani.yacoub)

Updated

10 months ago
Flags: needinfo?(jthomas)

Comment 19

10 months ago
Posted file signed.9006397.xpi (obsolete) —
Signed file attached. Please test.
Flags: needinfo?(wezhou)
Assignee

Comment 20

10 months ago
(In reply to :wezhou from comment #19)
> Created attachment 9006637 [details]
> signed.9006397.xpi
> 
> Signed file attached. Please test.

Rehan, would you mind staging this on the release-sysaddon channel (v2.0)? Thanks!
Flags: needinfo?(rdalal)
This is on the tests channel (release-sysaddon) and pending sign off on release.
Flags: needinfo?(rdalal)
This is currently set up in balrog for 61.0(.x).  Is that what we want, or should it be offered to 62.0(.x) too?
Flags: needinfo?(rhelmer)
Assignee

Comment 23

10 months ago
(In reply to Julien Cristau [:jcristau] from comment #22)
> This is currently set up in balrog for 61.0(.x).  Is that what we want, or
> should it be offered to 62.0(.x) too?

We should do 62 at this point too, thanks!
Flags: needinfo?(rhelmer)
(In reply to Robert Helmer [:rhelmer] from comment #23)
> (In reply to Julien Cristau [:jcristau] from comment #22)
> > This is currently set up in balrog for 61.0(.x).  Is that what we want, or
> > should it be offered to 62.0(.x) too?
> 
> We should do 62 at this point too, thanks!

I don't seem to have permissions to do that in balrog.  Rehan?
Flags: needinfo?(rdalal)
This is now on the test channel (release-sysaddon) for 61.* and 62.* and pending sign off for both versions on release.
Flags: needinfo?(rdalal)

Updated

10 months ago
Flags: needinfo?(jcristau)
Can we get RelMan sign off? Many thanks.
Hani, what is the status of testing on 61 and 62?

It also looks like https://github.com/mozilla/one-off-system-add-ons/pull/123#discussion_r215205711 didn't get to a resolution?
Flags: needinfo?(hani.yacoub)
Flags: needinfo?(rhelmer)
Assignee

Comment 28

10 months ago
(In reply to Julien Cristau [:jcristau] from comment #27)
> Hani, what is the status of testing on 61 and 62?
> 
> It also looks like
> https://github.com/mozilla/one-off-system-add-ons/pull/
> 123#discussion_r215205711 didn't get to a resolution?

Yes we should resolve this.

Georg, did you get a chance to see my responses to the GH PR ^?

It's quite easy to miss these sorts of responses in GH unfortunately :/
Flags: needinfo?(rhelmer) → needinfo?(gfritzsche)

Comment 29

10 months ago
Posted image 62.0.png (obsolete) —
I am attaching 2 screenshots comparing Firefox 62.0 and 61.0.2 results.
I followed the steps mentioned in comment 12 but without changing  "extensions.systemAddon.update.url" pref.

In firefox 62.0: "Telemetry coverage" is displayed in Firefox features in "about:support", and in the Network tab it shows a connection to "telemetry-coverage.m.o".

But in Firefox 61.0.2:  "Telemetry coverage" isn't displayed in Firefox features, and in the Network tab it shows a connection different than the one displayed in Firefox 62.
And this is displayed in the console:
1536320159663	addons.xpi	WARN	System add-on bug1487578@mozilla.org">telemetry-coverage-bug1487578@mozilla.org isn't compatible with the application.
Flags: needinfo?(hani.yacoub)

Comment 30

10 months ago
Posted image 61.0.2.png (obsolete) —
The new version says "<em:minVersion>61.0.*</em:minVersion>" which is still > 61.0.2.  Robert, I think this needs to be
"<em:minVersion>61.0</em:minVersion>".
Flags: needinfo?(rhelmer)
(In reply to Robert Helmer [:rhelmer] from comment #28)
> (In reply to Julien Cristau [:jcristau] from comment #27)
> > Hani, what is the status of testing on 61 and 62?
> > 
> > It also looks like
> > https://github.com/mozilla/one-off-system-add-ons/pull/
> > 123#discussion_r215205711 didn't get to a resolution?
> 
> Yes we should resolve this.
> 
> Georg, did you get a chance to see my responses to the GH PR ^?
> 
> It's quite easy to miss these sorts of responses in GH unfortunately :/

Indeed, thanks for flagging this.
I commented there, let me know if more is needed.
Flags: needinfo?(gfritzsche)
Assignee

Comment 33

10 months ago
(In reply to Julien Cristau [:jcristau] from comment #31)
> The new version says "<em:minVersion>61.0.*</em:minVersion>" which is still
> > 61.0.2.  Robert, I think this needs to be
> "<em:minVersion>61.0</em:minVersion>".

Fixed in the github review (we ended up setting it pretty far back as we may want to ship to older clients too)
Flags: needinfo?(rhelmer)
Assignee

Comment 34

10 months ago
Please sign this Telemetry Coverage system add-on update. Thanks!
Attachment #9005525 - Attachment is obsolete: true
Attachment #9005721 - Attachment is obsolete: true
Attachment #9006228 - Attachment is obsolete: true
Attachment #9006229 - Attachment is obsolete: true
Attachment #9006397 - Attachment is obsolete: true
Attachment #9006637 - Attachment is obsolete: true
Attachment #9007185 - Attachment is obsolete: true
Attachment #9007186 - Attachment is obsolete: true
Flags: needinfo?(wezhou)
Flags: needinfo?(jthomas)
Assignee

Comment 35

10 months ago
Comment on attachment 9008108 [details]
System add-on update for telemetry coverage (v3.0), unsigned

Sorry going to re-attach a new version momentarily.
Attachment #9008108 - Attachment is obsolete: true
Flags: needinfo?(wezhou)
Flags: needinfo?(jthomas)
Assignee

Comment 36

10 months ago
Please sign this Telemetry Coverage SAO update. Thank you!
Flags: needinfo?(wezhou)
Flags: needinfo?(jthomas)

Comment 37

10 months ago
Posted file signed.9008113.xpi (obsolete) —
Signed file attached. Please test.
Flags: needinfo?(wezhou)
Flags: needinfo?(jthomas)
Rehan can you set this up on release-sysaddon?
Flags: needinfo?(jcristau) → needinfo?(rdalal)
This is on the test channel (release-sysaddon) for 61.* and 62.* and pending sign off on release.
Flags: needinfo?(rdalal)
Assignee

Comment 40

10 months ago
We went through one round of testing on this and have made some minor changes in the meantime, should we do another? Can you arrange this or do I file a new PI request or? :) Thanks!
Flags: needinfo?(ryanvm)
Flags: needinfo?(jcristau)
Assignee

Comment 41

10 months ago
(In reply to Robert Helmer [:rhelmer] from comment #40)
> We went through one round of testing on this and have made some minor
> changes in the meantime, should we do another? Can you arrange this or do I
> file a new PI request or? :) Thanks!

Hani, could you please test this again? Latest version (4.0) is on the release-sysaddon channel and signed for Release, thanks!
Flags: needinfo?(hani.yacoub)
Assignee

Updated

10 months ago
Flags: needinfo?(ryanvm)
Flags: needinfo?(jcristau)

Comment 42

9 months ago
I tested on Firefox 62.0 and Firefox 61.0.2 and changed the update channel to "release-sysaddon" and the results are: 
"Telemetry coverage" is displayed in Firefox features in "about:support", and in the Network tab it shows a connection to "https://telemetry-coverage.mozilla.org/submit/coverage/coverage/1".

I tested on Firefox 62.0 and Firefox 61.0.2 WITHOUT changing the update channel and the results are:
"Telemetry coverage" isn't displayed in Firefox features, and in the Network tab it shows a connection different than the one displayed in Firefox 62.


Is this expected?
Flags: needinfo?(hani.yacoub) → needinfo?(rhelmer)
That sounds about right to me, though I don't know what you mean by "it shows a connection different than the one displayed in Firefox 62".

Did you also check the contents of the telemetry-coverage ping when data collection is turned on/off?
Flags: needinfo?(hani.yacoub)

Comment 44

9 months ago
I meant that it doesn't display a connection to "https://telemetry-coverage.mozilla.org/submit/coverage/coverage/1".

I didn't check that, I wasn't even sure how to do that until I asked a colleague who have experience working with telemetry.

From "about:preferences" in "Firefox Data Collection and Use" section, I left the first box checked and then I verified in about:telemetry to check the contents but I didn't see any of the params mentioned in comment 1. (Please see screenshot Data On.png)

Then I tried on a new profile, I unchecked the first box from "Firefox Data Collection and Use" section, and the results were as before, "Telemetry coverage" is displayed in Firefox features in "about:support", and in the Network tab it shows a connection to "https://telemetry-coverage.mozilla.org/submit/coverage/coverage/1".
In about:telemetry I didn't see any of the params mentioned before. (Please see screenshot Data Off.png)

I hope that what I just did is the same thing you were referring to.
Regards.
Flags: needinfo?(hani.yacoub) → needinfo?(jcristau)
(In reply to Hani Yacoub from comment #44)
> I meant that it doesn't display a connection to
> "https://telemetry-coverage.mozilla.org/submit/coverage/coverage/1".
> 
Right, that connection is added by the telemetry-coverage addon, so it's not expected to occur without it.

> I didn't check that, I wasn't even sure how to do that until I asked a
> colleague who have experience working with telemetry.
> 
> From "about:preferences" in "Firefox Data Collection and Use" section, I
> left the first box checked and then I verified in about:telemetry to check
> the contents but I didn't see any of the params mentioned in comment 1.
> (Please see screenshot Data On.png)
> 
> Then I tried on a new profile, I unchecked the first box from "Firefox Data
> Collection and Use" section, and the results were as before, "Telemetry
> coverage" is displayed in Firefox features in "about:support", and in the
> Network tab it shows a connection to
> "https://telemetry-coverage.mozilla.org/submit/coverage/coverage/1".
> In about:telemetry I didn't see any of the params mentioned before. (Please
> see screenshot Data Off.png)
> 
OK, nevermind me, it looks like the contents of the ping is not visible in the browser console so it's not so easy to check.  I tested 61.0.2 with telemetry on and off, with/without the opt-out pref, with/without the doctored telemetry clientID, and am satisfied things work as expected.
Flags: needinfo?(jcristau)
Actually, scratch that.  The browser toolbox shows the request body:

-----------------------------30901277611907454981161232464
Content-Disposition: form-data; name="telemetry_enabled"

[object Object]
-----------------------------30901277611907454981161232464--

Robert, it looks like this is missing a call to JSON.stringify when adding the payload to the formdata?
Assignee

Comment 47

9 months ago
Please sign this SAO update (5.0), thanks!
Attachment #9008113 - Attachment is obsolete: true
Attachment #9008227 - Attachment is obsolete: true
Flags: needinfo?(wezhou)
Flags: needinfo?(rhelmer)
Flags: needinfo?(jthomas)

Comment 48

9 months ago
Posted file signed.9008786.xpi (obsolete) —
Signed file attached. Please test.
Flags: needinfo?(wezhou)
Flags: needinfo?(jthomas)
Assignee

Comment 49

9 months ago
Comment on attachment 9008830 [details]
signed.9008786.xpi

Rehan, could you please put this one (v5) up on the release-sysaddon channel? Thanks!
Flags: needinfo?(rdalal)
This is now on the test channel (release-sysaddon) for 61.* and 62.*

It is pending signoff for 61.* and 62.* on release.
Flags: needinfo?(rdalal)
We've managed to conduct a test run on the release-sysaddon channel. 

We successfully installed the addon and verified a ping to  https://telemetry-coverage.mozilla.org with the following content:

-----------------------------2176630147023
Content-Disposition: form-data; name="telemetry_enabled"

{"appVersion":"62.0","appUpdateChannel":"release-sysaddon","osName":"WINNT","osVersion":"10.0","telemetryEnabled":1}
-----------------------------2176630147023--


Once the pref "toolkit.telemetry.coverage.opt-out" is created and set to true, no ping is sent to "toolkit.telemetry.coverage.opt-out".

Let me know if you have any questions.
(In reply to Stefan [:StefanG_QA] from comment #51)
> Let me know if you have any questions.

I don't see these pings hitting our downstream processing systems. I'm investigating why this is. A cursory inspection suggests that the pings are being sent with an incorrect URL path:

/submit/coverage/coverage/1

when there should be a UUID in there e.g. 

/submit/coverage/coverage/1/245be947-c848-4205-8deb-e7382f4890a8
Assignee

Comment 53

9 months ago
(In reply to Wesley Dawson [:whd] from comment #52)
> (In reply to Stefan [:StefanG_QA] from comment #51)
> > Let me know if you have any questions.
> 
> I don't see these pings hitting our downstream processing systems. I'm
> investigating why this is. A cursory inspection suggests that the pings are
> being sent with an incorrect URL path:
> 
> /submit/coverage/coverage/1
> 
> when there should be a UUID in there e.g. 
> 
> /submit/coverage/coverage/1/245be947-c848-4205-8deb-e7382f4890a8

After discussion we decided not to do a UUID, as this is one-shot and no retry mechanism etc.

Mark, did I get that right^?
Flags: needinfo?(mreid)
Assignee

Comment 54

9 months ago
(To clarify we'd like to add this in the future but we wanted to simplify this to get it going a little sooner)
(In reply to Robert Helmer [:rhelmer] from comment #53)
> (In reply to Wesley Dawson [:whd] from comment #52)
> 
> After discussion we decided not to do a UUID, as this is one-shot and no
> retry mechanism etc.
> 
> Mark, did I get that right^?

There may have been a miscommunication because we definitely need a unique document ID in the URL path. Our ingestion systems consider a submission without one to be invalid.
(In reply to Wesley Dawson [:whd] from comment #55)
> (In reply to Robert Helmer [:rhelmer] from comment #53)
> > (In reply to Wesley Dawson [:whd] from comment #52)
> > 
> > After discussion we decided not to do a UUID, as this is one-shot and no
> > retry mechanism etc.
> > 
> > Mark, did I get that right^?
> 
> There may have been a miscommunication because we definitely need a unique
> document ID in the URL path. Our ingestion systems consider a submission
> without one to be invalid.

Sorry, I thought the document ID was optional. Rob, can we still add that?
Flags: needinfo?(mreid) → needinfo?(rhelmer)
Assignee

Comment 57

9 months ago
(In reply to Mark Reid [:mreid] from comment #56)
> (In reply to Wesley Dawson [:whd] from comment #55)
> > (In reply to Robert Helmer [:rhelmer] from comment #53)
> > > (In reply to Wesley Dawson [:whd] from comment #52)
> > > 
> > > After discussion we decided not to do a UUID, as this is one-shot and no
> > > retry mechanism etc.
> > > 
> > > Mark, did I get that right^?
> > 
> > There may have been a miscommunication because we definitely need a unique
> > document ID in the URL path. Our ingestion systems consider a submission
> > without one to be invalid.
> 
> Sorry, I thought the document ID was optional. Rob, can we still add that?

We can, will need to go through another signing+staging round and possible QA, I'll get that started.
Flags: needinfo?(rhelmer)
Assignee

Comment 58

9 months ago
Would you mind reviewing https://github.com/mozilla/one-off-system-add-ons/pull/123 and making sure the submission bits look ok?

Specifically this const at the top:

  const REPORTING_ENDPOINT = "https://telemetry-coverage.mozilla.org/submit/coverage/1";

Which gets a unique UUID for each submission that looks like:

  let endpoint = `${REPORTING_ENDPOINT}/${uuid}`;

And the payload which is:

  const payload = {
    "appVersion": Services.appinfo.version,
    "appUpdateChannel": UpdateUtils.getUpdateChannel(false),
    "osName": Services.appinfo.OS,
    "osVersion": Services.sysinfo.getProperty("version"),
    "telemetryEnabled": enabled | 0
  };


Thanks!
Flags: needinfo?(whd)
Flags: needinfo?(mreid)
Assignee

Comment 59

9 months ago
Could you please sign this (v6) of the Telemetry Coverage SAO update? Thanks!
Attachment #9008786 - Attachment is obsolete: true
Attachment #9008830 - Attachment is obsolete: true
(In reply to Robert Helmer [:rhelmer] from comment #58)
> Would you mind reviewing
> https://github.com/mozilla/one-off-system-add-ons/pull/123 and making sure
> the submission bits look ok?
> 
> Specifically this const at the top:
> 
>   const REPORTING_ENDPOINT =
> "https://telemetry-coverage.mozilla.org/submit/coverage/1";
> 
> Which gets a unique UUID for each submission that looks like:
> 
>   let endpoint = `${REPORTING_ENDPOINT}/${uuid}`;
> 
> And the payload which is:
> 
>   const payload = {
>     "appVersion": Services.appinfo.version,
>     "appUpdateChannel": UpdateUtils.getUpdateChannel(false),
>     "osName": Services.appinfo.OS,
>     "osVersion": Services.sysinfo.getProperty("version"),
>     "telemetryEnabled": enabled | 0
>   };
> 
> 
> Thanks!

This looks good to me.
Flags: needinfo?(whd)
Flags: needinfo?(mreid)

Comment 61

9 months ago
Posted image headers.png (obsolete) —
I tested with the unsigned version of "System add-on update for telemetry coverage (v6.0)" on the latest nightly and the results are: 

- I can confirm that the pings are being sent with an correct URL path including UUID. (ex: https://telemetry-coverage.mozilla.org/submit/coverage/1/e2bd8234-644d-4992-8689-db7c77e80a74) Please see screenshot (heards.png).

- The payload is:
-----------------------------25441615019225
Content-Disposition: form-data; name="telemetry_enabled"

{"appVersion":"64.0a1","appUpdateChannel":"nightly","osName":"WINNT","osVersion":"10.0","telemetryEnabled":1}
-----------------------------25441615019225--
Assignee

Comment 62

9 months ago
(In reply to Hani Yacoub from comment #61)
> Created attachment 9009114 [details]
> headers.png
> 
> I tested with the unsigned version of "System add-on update for telemetry
> coverage (v6.0)" on the latest nightly and the results are: 
> 
> - I can confirm that the pings are being sent with an correct URL path
> including UUID. (ex:
> https://telemetry-coverage.mozilla.org/submit/coverage/1/e2bd8234-644d-4992-
> 8689-db7c77e80a74) Please see screenshot (heards.png).
> 
> - The payload is:
> -----------------------------25441615019225
> Content-Disposition: form-data; name="telemetry_enabled"
> 
> {"appVersion":"64.0a1","appUpdateChannel":"nightly","osName":"WINNT",
> "osVersion":"10.0","telemetryEnabled":1}
> -----------------------------25441615019225--

Wesley, could you please confirm if you saw this come into Telemetry? You should have seen one from me too from last night :)

Thanks!
Flags: needinfo?(whd)
(In reply to Robert Helmer [:rhelmer] from comment #62)
> (In reply to Hani Yacoub from comment #61)

> Wesley, could you please confirm if you saw this come into Telemetry? You
> should have seen one from me too from last night :)
> 
> Thanks!

I should have checked this a little more closely. I remember skimming the PR and saw some notes about the value of telemetryEnabled being boolean (https://github.com/mozilla/one-off-system-add-ons/pull/123/files/df0d5e73804e198254cc19ad8c3ec70ef594ffca) but the code actually does the opposite, and we've got it the other way around in our schemas https://github.com/mozilla-services/mozilla-pipeline-schemas/pull/198/files.

However, this is trivial to fix pipeline-side so in the interest of time we should probably update our schema. I'm discussing this with :mreid.
Flags: needinfo?(whd)
https://github.com/mozilla-services/mozilla-pipeline-schemas/pull/202/files has been deployed, so we should be good to go. :rhelmer can you send another ping to confirm?
Flags: needinfo?(rhelmer)
Assignee

Comment 65

9 months ago
(In reply to Wesley Dawson [:whd] from comment #64)
> https://github.com/mozilla-services/mozilla-pipeline-schemas/pull/202/files
> has been deployed, so we should be good to go. :rhelmer can you send another
> ping to confirm?

Done, posted to endpoint: https://telemetry-coverage.mozilla.org/submit/coverage/1/5be7750c-86e6-9f41-8dd5-561583f2d7c7

With payload:

{ appVersion: "64.0a1",
  appUpdateChannel: "nightly",
  osName: "Darwin",
  osVersion: "17.7.0",
  telemetryEnabled: 1
}
Flags: needinfo?(rhelmer)
(In reply to Robert Helmer [:rhelmer] from comment #65)
> (In reply to Wesley Dawson [:whd] from comment #64)
> > https://github.com/mozilla-services/mozilla-pipeline-schemas/pull/202/files
> > has been deployed, so we should be good to go. :rhelmer can you send another
> > ping to confirm?
> 
> Done, posted to endpoint:
> https://telemetry-coverage.mozilla.org/submit/coverage/1/5be7750c-86e6-9f41-
> 8dd5-561583f2d7c7
> 
> With payload:
> 
> { appVersion: "64.0a1",
>   appUpdateChannel: "nightly",
>   osName: "Darwin",
>   osVersion: "17.7.0",
>   telemetryEnabled: 1
> }

It turns out I've missed another thing in comment #62. We have should have both a namespace and doctype of coverage per https://bugzilla.mozilla.org/show_bug.cgi?id=1480194#c20, so the reporting endpoint should be:

const REPORTING_ENDPOINT = "https://telemetry-coverage.mozilla.org/submit/coverage/coverage/1";
Assignee

Comment 67

9 months ago
(In reply to Wesley Dawson [:whd] from comment #66)
> (In reply to Robert Helmer [:rhelmer] from comment #65)
> > (In reply to Wesley Dawson [:whd] from comment #64)
> > > https://github.com/mozilla-services/mozilla-pipeline-schemas/pull/202/files
> > > has been deployed, so we should be good to go. :rhelmer can you send another
> > > ping to confirm?
> > 
> > Done, posted to endpoint:
> > https://telemetry-coverage.mozilla.org/submit/coverage/1/5be7750c-86e6-9f41-
> > 8dd5-561583f2d7c7
> > 
> > With payload:
> > 
> > { appVersion: "64.0a1",
> >   appUpdateChannel: "nightly",
> >   osName: "Darwin",
> >   osVersion: "17.7.0",
> >   telemetryEnabled: 1
> > }
> 
> It turns out I've missed another thing in comment #62. We have should have
> both a namespace and doctype of coverage per
> https://bugzilla.mozilla.org/show_bug.cgi?id=1480194#c20, so the reporting
> endpoint should be:
> 
> const REPORTING_ENDPOINT =
> "https://telemetry-coverage.mozilla.org/submit/coverage/coverage/1";

OK just sent another test, same payload w/ this endpoint:

https://telemetry-coverage.mozilla.org/submit/coverage/coverage/1/dd43518f-9a50-684b-bc40-ab6ef878238e
Assignee

Comment 68

9 months ago
:whd and I confirmed that this works and makes it through the pipeline:

https://pipeline-cep.prod.mozaws.net/dashboard_output/analysis.moz_generic_coverage_coverage_1_pings.submissions.json

Please sign this SAO update, thanks!
Attachment #9008986 - Attachment is obsolete: true
Attachment #9009114 - Attachment is obsolete: true
Flags: needinfo?(wezhou)

Comment 69

9 months ago
Posted file signed.9009228.xpi
Signed file attached. Please test.
Flags: needinfo?(wezhou)
Assignee

Comment 70

9 months ago
(In reply to :wezhou from comment #69)
> Created attachment 9009328 [details]
> signed.9009228.xpi
> 
> Signed file attached. Please test.

Please stage this on the release-sysaddon channel - thanks!
Flags: needinfo?(rdalal)
This is on the test channel (release-sysaddon) and pending sign off on release.
Flags: needinfo?(rdalal)
Assignee

Comment 72

9 months ago
The only changes since the last round of QA have been to change the way we deliver the payload and the endpoint URL.

Are we ready to ship this? Thanks!
Flags: needinfo?(ryanvm)
Flags: needinfo?(jcristau)
Assignee

Comment 73

9 months ago
(In reply to Robert Helmer [:rhelmer] from comment #72)
> The only changes since the last round of QA have been to change the way we
> deliver the payload and the endpoint URL.
> 
> Are we ready to ship this? Thanks!

Here's the diff comparing v5 and v7 from http://archive.mozilla.org/pub/system-addons/telemetry-coverage-bug1487578/:

diff -u -r t5/bootstrap.js t7/bootstrap.js
--- t5/bootstrap.js	2018-09-13 09:04:17.000000000 -0700
+++ t7/bootstrap.js	2018-09-14 12:09:42.000000000 -0700
@@ -48,14 +48,17 @@
     "telemetryEnabled": enabled | 0
   };
 
-  let formData = new FormData();
-  formData.append("telemetry_enabled", JSON.stringify(payload));
+  let uuidGenerator = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
+  // generateUUID() gives a UUID surrounded by {...}, slice them off.
+  let uuid = uuidGenerator.generateUUID().toString().slice(1, -1);
 
-  debug(`posting to endpoint ${REPORTING_ENDPOINT} with payload:`, payload);
+  let endpoint = `${REPORTING_ENDPOINT}/${uuid}`;
 
-  await fetch(REPORTING_ENDPOINT, {
+  debug(`posting to endpoint ${endpoint} with payload:`, payload);
+
+  await fetch(endpoint, {
     method: "PUT",
-    body: formData
+    body: JSON.stringify(payload),
   });
 }
 
diff -u -r t5/install.rdf t7/install.rdf
--- t5/install.rdf	2018-09-13 09:04:24.000000000 -0700
+++ t7/install.rdf	2018-09-14 12:10:59.000000000 -0700
@@ -2,7 +2,7 @@
 <RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns:em="http://www.mozilla.org/2004/em-rdf#">
   <Description about="urn:mozilla:install-manifest">
     <em:id>bug1487578@mozilla.org">telemetry-coverage-bug1487578@mozilla.org</em:id>
-    <em:version>5.0</em:version>
+    <em:version>7.0</em:version>
     <em:type>2</em:type>
     <em:bootstrap>true</em:bootstrap>
     <em:unpack>false</em:unpack>
We've managed to conduct another test run on the release-sysaddon channel using Fx62. 

Once the addon is installed, we verified a ping to https://telemetry-coverage.mozilla.org with the following content:

appUpdateChannel	release-sysaddon
appVersion	62.0
osName	WINNT
osVersion	6.1
telemetryEnabled	1


In addition if the pref "toolkit.telemetry.coverage.opt-out" is created and set to true, no ping is sent to "toolkit.telemetry.coverage.opt-out".
Assignee

Comment 75

9 months ago
(In reply to Stefan [:StefanG_QA] from comment #74)
> We've managed to conduct another test run on the release-sysaddon channel
> using Fx62. 
> 
> Once the addon is installed, we verified a ping to
> https://telemetry-coverage.mozilla.org with the following content:
> 
> appUpdateChannel	release-sysaddon
> appVersion	62.0
> osName	WINNT
> osVersion	6.1
> telemetryEnabled	1
> 
> 
> In addition if the pref "toolkit.telemetry.coverage.opt-out" is created and
> set to true, no ping is sent to "toolkit.telemetry.coverage.opt-out".

Thanks! These pings made it through the Telemetry pipeline too:

https://pipeline-cep.prod.mozaws.net/dashboard_output/analysis.moz_generic_coverage_coverage_1_pings.submissions.json

Julien/Ryan, this is awaiting sign-off in Balrog.
Flags: needinfo?(ryanvm)
Flags: needinfo?(jcristau)
I checked that a ping from a client with telemetry disabled does show up with "telemetryEnabled: 0" at the other end (all other pings look like they had telemetry on).

Slight concern that osVersion at least on Linux gives more information than we really need, but I've now signed off on the change for 61.* and 62.*, so this is live.
Assignee

Comment 77

9 months ago
(In reply to Julien Cristau [:jcristau] from comment #76)
> I checked that a ping from a client with telemetry disabled does show up
> with "telemetryEnabled: 0" at the other end (all other pings look like they
> had telemetry on).
> 
> Slight concern that osVersion at least on Linux gives more information than
> we really need, but I've now signed off on the change for 61.* and 62.*, so
> this is live.

Thanks! It does seem that Services.sysinfo.getProperty("version") using the kernel version isn't quite consistent with what is reported on Windows/macOS (major OS version only). I'll see if we have anything more generic, something like distro + version would probably be more useful for tracking down problems than the kernel version anyway (although from a quick look I'm not sure if Firefox gathers that info).
Assignee

Comment 78

9 months ago
(In reply to Robert Helmer [:rhelmer] from comment #77)
> (In reply to Julien Cristau [:jcristau] from comment #76)
> > I checked that a ping from a client with telemetry disabled does show up
> > with "telemetryEnabled: 0" at the other end (all other pings look like they
> > had telemetry on).
> > 
> > Slight concern that osVersion at least on Linux gives more information than
> > we really need, but I've now signed off on the change for 61.* and 62.*, so
> > this is live.
> 
> Thanks! It does seem that Services.sysinfo.getProperty("version") using the
> kernel version isn't quite consistent with what is reported on Windows/macOS
> (major OS version only). I'll see if we have anything more generic,
> something like distro + version would probably be more useful for tracking
> down problems than the kernel version anyway (although from a quick look I'm
> not sure if Firefox gathers that info).

Actually I take that back - looks like this is reporting kernel version which is at least consistent, and on Windows/macOS lets us map that to an OS version at least.
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Assignee

Comment 79

9 months ago
I see data coming in that looks reasonable, I think we're done for now. Thanks everyone!
You need to log in before you can comment on or make changes to this bug.