7.17 KB, patch
|Details | Diff | Splinter Review|
16.76 KB, text/plain
1.63 KB, patch
|Details | Diff | Splinter Review|
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.
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.
Updated to put the common code in a js module DataCollectionUtils.jsm
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-
Note we should uplift this to aurora/beta once it lands.
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.
(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.
Removed the 2 single-use "PREFS" constants, put the strings inline.
Comment on attachment 591575 [details] [diff] [review] Patch to add "appUpdateChannel" to telemetry submissions Carrying forward Gavin's r+.
Attachment #591575 - Flags: review+
Creating a proper module to extract common code is tracked by bug 721165.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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.
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.
(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?
(In reply to Gavin Sharp (use email@example.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
Whiteboard: [Telemetry:P1] → [Telemetry:P1][qa+]
Based on comment #25, setting status-firefox11: --- → verified.
You need to log in before you can comment on or make changes to this bug.