Last Comment Bug 762590 - Telemetry.canSend check doesn't prevent pings from being sent
: Telemetry.canSend check doesn't prevent pings from being sent
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nathan Froyd [:froydnj]
:
:
Mentors:
Depends on: 762620
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-07 10:55 PDT by (dormant account)
Modified: 2012-06-23 10:10 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
be more careful with canSend logic (1.23 KB, patch)
2012-06-07 12:25 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description (dormant account) 2012-06-07 10:55:57 PDT
10:48 <@taras> looking at the code
10:48 <@taras> the canSend check
10:48 <@taras> causes us to still send the data
10:48 <@taras> but as a test-ping :)
10:51 <@taras> so the 404, wont be noticed
10:51 <@taras> 1/second thing is weird though
10:51 <@taras> must be some coincidence in our browser-idle scheduling + when devs go to work
Comment 1 Nathan Froyd [:froydnj] 2012-06-07 12:25:34 PDT
Created attachment 631080 [details] [diff] [review]
be more careful with canSend logic
Comment 2 (dormant account) 2012-06-07 14:19:32 PDT
Comment on attachment 631080 [details] [diff] [review]
be more careful with canSend logic

Logic one can actually follow
Comment 3 (dormant account) 2012-06-07 14:19:51 PDT
Lets uplift this into aurora/beta
Comment 5 Nathan Froyd [:froydnj] 2012-06-07 14:49:25 PDT
Comment on attachment 631080 [details] [diff] [review]
be more careful with canSend logic

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 722240
User impact if declined: Decreased battery life and excess internet traffic.
Testing completed (on m-c, etc.): Going on to m-c tomorrow, 8 June.
Risk to taking this patch (and alternatives if risky): Always a risk with telemetry patches that something can go wrong, but this seems pretty safe.
String or UUID changes made by this patch: None.
Comment 6 Nathan Froyd [:froydnj] 2012-06-07 17:20:48 PDT
Actually, how much of a problem is this, really?  Telemetry.canSend should only ever be false for *non*-official builds (developer builds, Iceweasel, etc.) so this shouldn't be a problem on our official builds.
Comment 7 Nathan Froyd [:froydnj] 2012-06-07 17:22:09 PDT
That is, bug 758193 would seem to be the aurora/beta candidate here, not this bug.
Comment 8 Nathan Froyd [:froydnj] 2012-06-07 18:06:09 PDT
Comment on attachment 631080 [details] [diff] [review]
be more careful with canSend logic

Removing a? flags until we've discussed comment 6 and comment 7 in some more detail.
Comment 9 (dormant account) 2012-06-07 22:00:50 PDT
(In reply to Nathan Froyd (:froydnj) from comment #6)
> Actually, how much of a problem is this, really?  Telemetry.canSend should
> only ever be false for *non*-official builds (developer builds, Iceweasel,
> etc.) so this shouldn't be a problem on our official builds.

It's not much of a problem. Fennec team is doing development on the beta branch, so it would reduce traffic from developer devices. Indeed, there isn't any impact on official builds. Beta/aurora uplift is a nice to have.
Comment 10 Graeme McCutcheon [:graememcc] 2012-06-08 04:19:20 PDT
https://hg.mozilla.org/mozilla-central/rev/ea069bf72d62

(Merged by Ed Morley)
Comment 11 Nathan Froyd [:froydnj] 2012-06-08 06:54:01 PDT
Comment on attachment 631080 [details] [diff] [review]
be more careful with canSend logic

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 722240
User impact if declined: Decreased battery life, extra data usage.  Note that per comment 9, this is only a concern for Mozilla developers if they are developing off aurora/beta.
Testing completed (on m-c, etc.): Local testing, on m-c as of today.
Risk to taking this patch (and alternatives if risky): Epsilon.
String or UUID changes made by this patch: None.
Comment 12 Alex Keybl [:akeybl] 2012-06-11 14:09:25 PDT
Comment on attachment 631080 [details] [diff] [review]
be more careful with canSend logic

Won't affect official builds - approved for branches.
Comment 14 Gian-Carlo Pascutto [:gcp] 2012-06-12 09:08:28 PDT
To clarify based on a discussion on IRC: This *DOES* affect official builds because Fennec (XUL and Mobile) have (unintentionally it seems) have had telemetry disabled for a while.

With the bug, these devices were still able to send test-pings (disastrously so due to bug 762620). With the fix, the official builds for those devices will be affected and will no longer send any telemetry (but this is what we want).

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