720785 - Report channel in Telemetry

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Daniel Einspanjer [:dre] [:deinspanjer]]

Without the channel (release, default, beta, aurora, nightly) (%CHANNEL% in config variables), we can't tell beta submissions apart from release unless they happen to respin the builds for release, which is not a guarantee.

There are questions being asked of the Telemetry data that make this a necessity to quickly put in.

Comment 1

[Mark Reid [:mreid]]

Attachment: Patch to add "appUpdateChannel" to telemetry submissions

This patch copies the code from toolkit/mozapps/extensions/nsBlocklistService.js

Extracting it into a separate jsm would be useful to avoid code duplication.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Reid from comment #1)
> Extracting it into a separate jsm would be useful to avoid code duplication.

Can we just do that? Doesn't seem like it would be hard.

Comment 3

[Mark Reid [:mreid]]

Attachment: Extract code from nsBlocklistService.js to calculate the update channel, add that data to Telemetry

Updated to put the common code in a js module DataCollectionUtils.jsm

Comment 4

[Pedro Alves]

Can someone show a sample of the resulting json?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 591222 [details] [diff] [review]
Extract code from nsBlocklistService.js to calculate the update channel, add that data to Telemetry

This looks good, but I would just name the module UpdateChannel, put it in toolkit/content, and then just have it just export a lazy getter that returns the string value of the update channel (i.e. make this a simple single-purpose module), unless you have plans to add additional things to it.

You can then also use it in toolkit/mozapps/extensions/nsBlocklistService.js and toolkit/components/urlformatter/nsURLFormatter.js.

You're removing the only use of PREF_APP_UPDATE_CHANNEL in nsBlockListService.js but not removing it's definition (this same comment probably also applies to other files once they use this).

Comment 6

[(dormant account)]

Note we should uplift this to aurora/beta once it lands.

Comment 7

[Mark Reid [:mreid]]

I was planning to add more functionality to the library, there are a few other duplicated pieces of code in the Metrics Data Ping (bug 718067) that would be nice to treat the same way.  

Should I still change to a lazy getter?

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Taras Glek (:taras) from comment #6)
> Note we should uplift this to aurora/beta once it lands.

Once this lands on M-C we'll set the flags. I can champion this on the channel call.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

If the intent is to backport, let's go with the original patch that just duplicates the code. For trunk, we can investigate the module approach in more detail.

Comment 10

[Mark Reid [:mreid]]

Attachment: Sample Telemetry submission including the new "appUpdateChannel" key

(In reply to Pedro Alves from comment #4)
> Can someone show a sample of the resulting json?

See attachment. The new key is "appUpdateChannel"

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 591210 [details] [diff] [review]
Patch to add "appUpdateChannel" to telemetry submissions

I'd prefer avoiding the use of the PREF_ constants, it just obfuscates the code since they're only used once anyways.

Comment 12

[Mark Reid [:mreid]]

Attachment: Patch to add "appUpdateChannel" to telemetry submissions

Removed the 2 single-use "PREFS" constants, put the strings inline.

Comment 13

[Mark Reid [:mreid]]

Comment on attachment 591575 [details] [diff] [review]
Patch to add "appUpdateChannel" to telemetry submissions

Carrying forward Gavin's r+.

Comment 14

[Mark Reid [:mreid]]

Creating a proper module to extract common code is tracked by bug 721165.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6afce25c341f

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6afce25c341f

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

In reference to back porting (comment 6), is this ready to go or do we want to take the original patch as Gavin suggests (comment 9)?

Mark - Can you please add a comment with a risk assessment for use in the release drivers decision to take this on Aurora and Beta?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

The "original patch" is what we ended up landing on trunk (the more complicated fix was pushed off to bug 721165). Attachment 591575 [details] [diff] is ready for aurora/beta.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 591575 [details] [diff] [review]
Patch to add "appUpdateChannel" to telemetry submissions

[Approval Request Comment]
Testing completed (on m-c, etc.): I haven't done any testing. It'd be good to verify that the data from Nightly builds is suitable.

Risk to taking this patch (and alternatives if risky): Very low risk, just reads a pref and sticks it into the telemetry ping.

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 591575 [details] [diff] [review]
Patch to add "appUpdateChannel" to telemetry submissions

Approved for beta. Dropping Aurora request as Aurora is now 12 and this fix already rode the train to Aurora.

Comment 21

[Daniel Einspanjer [:dre] [:deinspanjer]]

(In reply to Lawrence Mandel [:lmandel] from comment #20)
> Approved for beta. ...

Does this require any further action from Mark?  The approval flag should prompt someone with commit access to put it in the beta branch in hg, right?

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

If Mark doesn't have commit rights than some else (Gavin or Ed?) can help out.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

I can land this patch on beta. It would be good to confirm that the telemetry data from Nightly builds with this patch is suitable. Daniel/Pedro, can you do that?

Comment 24

[(dormant account)]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #23)
> I can land this patch on beta. It would be good to confirm that the
> telemetry data from Nightly builds with this patch is suitable.
> Daniel/Pedro, can you do that?

13:55 < xstevens> yeah I saw different release channels when I was doing my changes

Comment 25

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4860aa403c4e

Comment 26

[Mihaela Velimiroviciu (:mihaelav)]

Based on comment #25, setting status-firefox11: --- → verified.