Last Comment Bug 720785 - Report channel in Telemetry
: Report channel in Telemetry
Status: VERIFIED FIXED
[Telemetry:P1][qa!]
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: 12 Branch
: All All
: P1 critical (vote)
: mozilla12
Assigned To: Mark Reid [:mreid]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-24 11:51 PST by Daniel Einspanjer [:dre] [:deinspanjer]
Modified: 2012-02-28 02:21 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
Patch to add "appUpdateChannel" to telemetry submissions (2.01 KB, patch)
2012-01-24 12:08 PST, Mark Reid [:mreid]
gavin.sharp: review+
Details | Diff | Review
Extract code from nsBlocklistService.js to calculate the update channel, add that data to Telemetry (7.17 KB, patch)
2012-01-24 12:35 PST, Mark Reid [:mreid]
gavin.sharp: review-
Details | Diff | Review
Sample Telemetry submission including the new "appUpdateChannel" key (16.76 KB, text/plain)
2012-01-25 06:45 PST, Mark Reid [:mreid]
no flags Details
Patch to add "appUpdateChannel" to telemetry submissions (1.63 KB, patch)
2012-01-25 11:56 PST, Mark Reid [:mreid]
mreid: review+
lmandel: approval‑mozilla‑beta+
Details | Diff | Review

Description Daniel Einspanjer [:dre] [:deinspanjer] 2012-01-24 11:51:12 PST
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] 2012-01-24 12:08:37 PST
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.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-24 12:10:13 PST
(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] 2012-01-24 12:35:12 PST
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
Comment 4 Pedro Alves 2012-01-24 12:41:35 PST
Can someone show a sample of the resulting json?
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-24 12:42:44 PST
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) 2012-01-24 12:46:32 PST
Note we should uplift this to aurora/beta once it lands.
Comment 7 Mark Reid [:mreid] 2012-01-24 12:48:28 PST
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) 2012-01-24 12:53:20 PST
(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: gavin@gavinsharp.com] 2012-01-24 13:22:38 PST
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] 2012-01-25 06:45:03 PST
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 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-25 11:48:08 PST
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] 2012-01-25 11:56:03 PST
Created attachment 591575 [details] [diff] [review]
Patch to add "appUpdateChannel" to telemetry submissions

Removed the 2 single-use "PREFS" constants, put the strings inline.
Comment 13 Mark Reid [:mreid] 2012-01-25 11:58:21 PST
Comment on attachment 591575 [details] [diff] [review]
Patch to add "appUpdateChannel" to telemetry submissions

Carrying forward Gavin's r+.
Comment 14 Mark Reid [:mreid] 2012-01-25 12:21:31 PST
Creating a proper module to extract common code is tracked by bug 721165.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-25 12:31:03 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/6afce25c341f
Comment 16 Ed Morley [:emorley] 2012-01-25 18:06:50 PST
https://hg.mozilla.org/mozilla-central/rev/6afce25c341f
Comment 17 Lawrence Mandel [:lmandel] (use needinfo) 2012-01-26 14:01:12 PST
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: gavin@gavinsharp.com] 2012-01-26 16:36:14 PST
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: gavin@gavinsharp.com] 2012-01-26 16:43:46 PST
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) 2012-01-31 14:45:04 PST
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] 2012-02-01 12:42:06 PST
(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) 2012-02-01 12:51:21 PST
If Mark doesn't have commit rights than some else (Gavin or Ed?) can help out.
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-01 12:57:32 PST
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) 2012-02-10 14:43:49 PST
(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
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-13 10:26:25 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/4860aa403c4e
Comment 26 Mihaela Velimiroviciu (:mihaelav) 2012-02-28 02:21:27 PST
Based on comment #25, setting status-firefox11: --- → verified.

Note You need to log in before you can comment on or make changes to this bug.