fix captive portal telemetry
Categories
(Firefox :: Messaging System, defect, P1)
Tracking
()
People
(Reporter: dmosedale, Assigned: dmosedale)
References
Details
Attachments
(1 file)
|
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-release+
|
Details | Review |
Filed in Messaging for tracking purposes; will move to Networking later.
In bug 1722834, telemetry was added for a few things, including when the captive-portal-login-success observer notification was fired, and when the captive portal login infobar was shown.
Unfortunately, after doing some redash analysis on our recent Nightly A:A experiment, we saw that the login-success notifications weren't always being sent.
After some code inspection, it became clear that the telemetry was being sent only when the captive portal infobar was made to disappear, and never if the captive portal infobar hadn't been shown. I've got a patch that moves it from an observer to the code into CaptiveDetect.jsm where the notification is being fired, which should be more correct and also less fragile, since it doesn't depend on when (for example) the observer gets registered.
I also took a look at the other telemetry, and the captive portal login infobar telemetry could be sent whenever the _showNotification method (which sometimes returns early) was called, meaning potentially too many times. I pushed this notification down into the _showNotification method to the spot after the notification is actually shown.
I'd like to get this in on Monday if at all possible, as ideally, I'd like to get this uplifted to 92 so that we can run a 92 release A:A experiment to validate our telemetry if necessary.
|
Assignee | |
Updated•4 years ago
|
|
|
Assignee | |
Comment 1•4 years ago
|
||
[Tracking Requested - why for this release]:
|
|
Assignee | |
Updated•4 years ago
|
|
|
Assignee | |
Comment 2•4 years ago
|
||
|
||
Comment 4•4 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 5•4 years ago
|
||
I've verified this (as much as can be verified without doing an experiment) by looking at the live telemetry tables and checking the telemetry sent by different builds on different days. August 31st isn't even over yet, and we're already seeing many more successful logins per day than in the older build, which is exactly what we would expect.
|
|
Assignee | |
Comment 6•4 years ago
•
|
||
Comment on attachment 9238444 [details]
Bug 1728057 - fix captive portal telemetry, r?nhnt11!
Beta/Release Uplift Approval Request
-
User impact if declined: The impact is not to end-users; but it will be more difficult and time-consuming to correctly size a revenue-related experiment to be run on Release 93. That said, it probably makes sense for this to be a ride-along if there's another release candidate; I'm not sure if it's worth generating a new release candidate for.
-
Is this code covered by automated tests?: Yes
-
Has the fix been verified in Nightly?: Yes
-
Needs manual test from QE?: No
-
If yes, steps to reproduce:
-
List of other uplifts needed: None
-
Risk to taking this patch: Low
-
Why is the change risky/not risky? (and alternatives if risky): There are no user-facing changes to the patch at all. The most critical telemetry data was already incorrect enough to not be usable; if for some reason it were to still be incorrect, we'd be no worse off. The patch itself is simple and low risk, moving a few bits of code to record telemetry from one spot to another.
-
String changes made/needed: None
|
|
Assignee | |
Comment 7•4 years ago
|
||
Additionally, this telemetry just got introduced in 92 for this set of experiments; noone else should be depending on it yet.
|
||
Comment 8•4 years ago
|
||
Comment on attachment 9238444 [details]
Bug 1728057 - fix captive portal telemetry, r?nhnt11!
Approved for 92.0rc2.
|
|
||
Comment 9•4 years ago
|
||
| bugherder uplift | ||
|
||
Updated•4 years ago
|
|
||
Comment 10•4 years ago
|
||
Marking this issue as Verified based on the work done for the feature. See QA-1191 for more details.
Description
•