The default bug view has changed. See this FAQ.

Report channel in Telemetry

VERIFIED FIXED in Firefox 11

Status

()

Toolkit
Telemetry
P1
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: dre, Assigned: mreid)

Tracking

12 Branch
mozilla12
Points:
---

Firefox Tracking Flags

(firefox11 verified)

Details

(Whiteboard: [Telemetry:P1][qa!])

Attachments

(3 attachments, 1 obsolete attachment)

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.
Whiteboard: [Telemetry:P1]
(Assignee)

Comment 1

5 years ago
Created attachment 591210 [details] [diff] [review]
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.
Assignee: nobody → mreid
Attachment #591210 - Flags: review?(taras.mozilla)
(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.
(Assignee)

Comment 3

5 years ago
Created attachment 591222 [details] [diff] [review]
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
Attachment #591210 - Attachment is obsolete: true
Attachment #591210 - Flags: review?(taras.mozilla)
Attachment #591222 - Flags: review?(gavin.sharp)

Comment 4

5 years ago
Can someone show a sample of the resulting json?
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).
Attachment #591222 - Flags: review?(gavin.sharp) → review-

Comment 6

5 years ago
Note we should uplift this to aurora/beta once it lands.
(Assignee)

Comment 7

5 years ago
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?
(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.
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.
(Assignee)

Comment 10

5 years ago
Created attachment 591446 [details]
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 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.
Attachment #591210 - Attachment is obsolete: false
Attachment #591210 - Flags: review+
(Assignee)

Comment 12

5 years ago
Created attachment 591575 [details] [diff] [review]
Patch to add "appUpdateChannel" to telemetry submissions

Removed the 2 single-use "PREFS" constants, put the strings inline.
Attachment #591210 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Carrying forward Gavin's r+.
Attachment #591575 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #591575 - Flags: checkin?(gavin.sharp)
Attachment #591575 - Flags: checkin?(gavin.sharp)
(Assignee)

Comment 14

5 years ago
Creating a proper module to extract common code is tracked by bug 721165.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/6afce25c341f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6afce25c341f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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?
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 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.
Attachment #591575 - Flags: approval-mozilla-beta?
Attachment #591575 - Flags: approval-mozilla-aurora?
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.
Attachment #591575 - Flags: approval-mozilla-beta?
Attachment #591575 - Flags: approval-mozilla-beta+
Attachment #591575 - Flags: approval-mozilla-aurora?
(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?
If Mark doesn't have commit rights than some else (Gavin or Ed?) can help out.
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

5 years ago
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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
https://hg.mozilla.org/releases/mozilla-beta/rev/4860aa403c4e
Status: RESOLVED → VERIFIED
status-firefox11: --- → fixed
Whiteboard: [Telemetry:P1] → [Telemetry:P1][qa+]
Based on comment #25, setting status-firefox11: --- → verified.
status-firefox11: fixed → verified
Whiteboard: [Telemetry:P1][qa+] → [Telemetry:P1][qa!]
You need to log in before you can comment on or make changes to this bug.