1119189 - FTE tour and room/call notifications are broken when Loop becomes unthrottled

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Alex Gibson [:agibson]]

As per our IRC conversation, when Loop is throttled notifications are not received, which breaks the Hello FTE tour.

11:18 Standard8: mikedeboer: just a thought, the throttle value changed yesterday as well (from unthrottled, back down to 10%)
11:19 mikedeboer: Standard8: well, I have the toolbar customized to show the button, and the throttling only affects that
11:22 mikedeboer: Standard8: yuck, you _are_ right
11:23 Standard8: mikedeboer: sometimes I hate being right
11:23 mikedeboer: agibson: you also need to set the 'loop.throttled2' pref to `false`
11:23 mikedeboer: and then it'll work
11:23 agibson: I see
11:24 agibson: so where does this leave regular users?
11:24 agibson: can we get this fixed?
11:24 mikedeboer: I *did* find another bug, but that doesn't stop it from working
11:24 agibson: ?
11:25 mikedeboer: agibson: well, technically, it's not a bug. The feature is disabled for for 90% of the beta population
11:25 Standard8: mikedeboer: but what about folks who have already customised it onto their toolbar?
11:25 Standard8: in 34
11:25 agibson: yeah, this will break the tour for those users
11:25 agibson: like myself
11:26 mikedeboer: agibson: the throttling mechanism messes up a whole bunch of things. It will fail to do random stuff like Standard8 mentions
11:26 mikedeboer: I'm not particularly fond of it, but myeah
11:26 agibson: so this is really a blocker for launching when Hello is throttled?
11:26 agibson: for the tour at least
11:26 mikedeboer: yup
11:26 agibson sigh
11:27 agibson: ok, I will email folks
11:28 mikedeboer: It's fixable, but I don't know if it'll be in time for the uplift of Beta -> release
11:28 mikedeboer: If uplifting a patch to beta is possible at this point, I don't know

-----

11:32 mikedeboer: agibson: in fact, I think it's rather easy to fix without putting additional load on the servers when throttled. So if you can file a bug, I'll attach a patch there and we can discuss how to move forward there.
11:33 agibson: mikedeboer: ok will do, many thanks

Comment 1

[Mike de Boer [:mikedeboer]]

Shell, can you add this bug to the backlog for this iteration, please?

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Beta patch: start listening to room events regardless of throttle settings

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Trunk patch: start listening to room events regardless of throttle settings

Comment 4

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:

Yesterday on the server side we turned the throttle back down to 10% such that most of the Fx35 population is now throttled (i.e.the Hello button is in the customization palette, instead of the main toolbar).  The tour does not work well when Hello is throttled.  So we need to take these patches in Fx35 or disable/scale-back the full tour in Fx35 in some way.  (We can also turn off the throttle, but the throttle is in place to protect overloading the servers on the first release of Hello.)

NOTE: There is no throttle in Fx36 (or later); so this will not affect future Hello releases.

Needinfo'ing gavin, ashughes, and lsblakk to make sure this bug is on their radar and so they can comment as needed on preferred ways forward.  The patches are currently up for review, so we're not ready to write an uplift request yet (and the uplift would only go to Fx35).  Thanks.

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Beta patch v2: don't let registration depend on throttling being disabled

Comment 6

[:Gavin Sharp [email: [e-mail]]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=024db3510c76

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8545926 [details] [diff] [review]
Beta patch v2: don't let registration depend on throttling being disabled

This is good, I've given it a few tests, and it seems to work fine.

Comment 8

[Mark Banner (:standard8)]

Just to expand on effects of this bug & testing for QA:

- Without the patch, the UI tour won't work properly - agibson has details on how to set this up.
- Without the patch, registration won't occur on startup, this means that users won't be notified of people joining the room, or incoming calls - unless they happen to open the Hello panel.

For testing:

You can monitor if registration occurs by starting with -jsconsole on the command line, and looking for entries such as:

POST https://loop.services.mozilla.com/v0/registration

(Note that sometimes the first time round on a profile, you need to toggle the "Net" button of the console on and off to see the network activity).

Main Tests:

1) Fresh Profile on Fx35, check that registration doesn't occur
2) Set loop.soft_start_ticket_number to '1' and restart FF, check that registration doesn't occur
3) With the UI tour set up per agibson, take the tour, check that you get messages leading you through room creation, getting notifications, and other people joining the room.
4) Restart the browser
5) Check that registration occurs, and you get notifications of people joining & leaving your room

Other tests:

- Fresh profile, generate a url in FF 34
- upgrade to 35, and change the soft start ticket number if necessary & restart
- Check that after 10 seconds after the restart you can receive calls from the url created in 34.

- Fresh profile, sign-in to FxA on Hello in 34
- Check you can receive a direct call (from another signed in browser on a different FxA account)
- Upgrade to 35 (doing the throttle dance if necessary)
- Check that after 10 seconds after the restart you can receive a direct call

Comment 9

[u279076]

If I understand this bug correctly it does not need to block release of Firefox 35 - it only needs to block completely unthrottling Hello for 35. However, if we plan to release with Loop fully unthrottled then this does need to block Firefox 35.

If a 35.0rc3 is required it would be best to have builds in QA by end of day today for our last night of overnight testing before the weekend.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #9)
> If I understand this bug correctly it does not need to block release of
> Firefox 35 - it only needs to block completely unthrottling Hello for 35.
> However, if we plan to release with Loop fully unthrottled then this does
> need to block Firefox 35.

No - this bug only affects throttled users. We'd actually be fine if we released unthrottled.

> If a 35.0rc3 is required it would be best to have builds in QA by end of day
> today for our last night of overnight testing before the weekend.

I think we're committed to this now, let's get the patch landed ASAP.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

From discussion with Standard8/MattN/agibson, I think this is the summary of impact:

For users who end up being throttled (i.e. X% of all loop users regardless of how they first became a loop user):
a) will never get room join/activity notifications
b) will not get direct calls/be marked as "online" unless they open the panel
c) will not be able to complete the tour (no room join notification), and so the tour will always pop up when the toolbarbutton is clicked

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8545926 [details] [diff] [review]
Beta patch v2: don't let registration depend on throttling being disabled

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: See comment 11
[Describe test coverage new/current, TBPL]: Patch only applies to beta. This is simple removal of a check, existing code is covered by unit/integration tests. Manual testing done by several people. See also comment 8 for more tests.
[Risks and why]: Low risk - stops disabling registration when throttled. The registration process has been designed to kick in only when required, and didn't need to be throttled separately. The throttle for 35 is only intended to limit the speed at which users see this on the toolbar.
[String/UUID change made/needed]: None

Comment 13

[Maire Reavy [:mreavy]]

Just to clarify for anyone testing this patch (expanding on Comment 11 and Comment 12):

As a throttled user, you should be able to move the Hello button from the customization palette to the toolbar and verify that you can (a) receive notifications and (b) receive direct calls -- even though you are still throttled -- as well as complete the tour (c).

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8545926 [details] [diff] [review]
Beta patch v2: don't let registration depend on throttling being disabled

[Triage Comment]
We need this on mozilla-release to do a 35.0 build 3, please uplift to both.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0003236c17c1
https://hg.mozilla.org/releases/mozilla-release/rev/0003236c17c1

Comment 16

[u279076]

Florin, assuming you get a 35.0 build 3 tonight, please make sure this gets extensive testing.

Comment 17

[Alex Gibson [:agibson]]

If someone wants to test the tour please make sure to set the following in about:config

1.) set loop.gettingStarted.url = https://www-demo2.allizom.org/%LOCALE%/firefox/%VERSION%/hello/start/ 
2.) set loop.gettingstarted.seen = false
3.) whitelist demo2 to use UiTour API by creating a pref called `browser.uitour.testingOrigins` and give it a value of https://www-demo2.allizom.org
4.) restart the browser
5.) Click the hello icon in the browser to open the Hello panel.

You should now see a blue button that says “Get Started”. Clicking this will open the tour and if all the above steps have worked you should see the FTE page on demo and the first door-hanger pointing at “Start a conversation”

Note if you want to retake the tour without resetting `loop.gettingstarted.seen`, the tour is also available via the ‘Tour’ link in the Hello panel gear menu

Comment 18

[Paul Silaghi, QA [:pauly]]

Verified fixed on FF 35 RC build 3 using the STR in comments 8, 13, 17.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Only affected 35, so this is FIXED now too.

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking as Verified based on comment 18.

Comment 21

[Matthew N. [:MattN]]

The existing summary I had was correct. The problem occurs when going from throttled => unthrottled.