762590 - Telemetry.canSend check doesn't prevent pings from being sent

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[(dormant account)]

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]]

Attachment: be more careful with canSend logic

Comment 2

[(dormant account)]

Comment on attachment 631080 [details] [diff] [review]
be more careful with canSend logic

Logic one can actually follow

Comment 3

[(dormant account)]

Lets uplift this into aurora/beta

Comment 4

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea069bf72d62

Comment 5

[Nathan Froyd [:froydnj]]

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]]

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]]

That is, bug 758193 would seem to be the aurora/beta candidate here, not this bug.

Comment 8

[Nathan Froyd [:froydnj]]

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)]

(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]]

https://hg.mozilla.org/mozilla-central/rev/ea069bf72d62

(Merged by Ed Morley)

Comment 11

[Nathan Froyd [:froydnj]]

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]]

Comment on attachment 631080 [details] [diff] [review]
be more careful with canSend logic

Won't affect official builds - approved for branches.

Comment 13

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/131e571d4959
https://hg.mozilla.org/releases/mozilla-beta/rev/7912483cf789

Comment 14

[Gian-Carlo Pascutto [:gcp]]

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).