If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Loop Client needs to send channel in call setup messages

VERIFIED FIXED in Firefox 34

Status

Hello (Loop)
Client
P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: abr, Assigned: standard8)

Tracking

unspecified
mozilla36
Points:
2
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox33 unaffected, firefox34+ verified, firefox35 verified, firefox36 verified)

Details

(Whiteboard: [landed in fx-team])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Client needs to send new "channel" field in call setup messages. See Bug 1033032 for details.
(Reporter)

Updated

3 years ago
Blocks: 1039743
Hi Adam - coming out of our conversation with Rob and Michael yesterday, what release should this target?
Flags: needinfo?(adam)
(Reporter)

Comment 2

3 years ago
Created attachment 8510311 [details] [diff] [review]
Add channel to POST /calls
(Reporter)

Comment 3

3 years ago
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #1)
> Hi Adam - coming out of our conversation with Rob and Michael yesterday,
> what release should this target?

Apparently, this slipped between the cracks. If you'd asked me back when we had the first conversations around the topic, I'd have said "33".

It's literally a three line change (see the attached patch) -- this simply needs to be tested, and I think it's a candidate to uplift to 34 and 35.
Flags: needinfo?(adam)
backlog: --- → Fx34+
(Assignee)

Updated

3 years ago
Assignee: nobody → standard8
Iteration: --- → 36.1
Target Milestone: --- → mozilla36
(Assignee)

Comment 4

3 years ago
Created attachment 8510492 [details] [diff] [review]
Add channel to POST calls for Loop to allow different servers based on the channel.

Updated patch to fix and add unit tests.
Attachment #8510492 - Flags: review?(dmose)
(Assignee)

Updated

3 years ago
Attachment #8510311 - Attachment is obsolete: true
Comment on attachment 8510492 [details] [diff] [review]
Add channel to POST calls for Loop to allow different servers based on the channel.

Review of attachment 8510492 [details] [diff] [review]:
-----------------------------------------------------------------

r=dmose
Attachment #8510492 - Flags: review?(dmose) → review+
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/e2f7534c7687
Points: --- → 2
Priority: -- → P1
(Assignee)

Updated

3 years ago
Whiteboard: [landed in fx-team]
(Assignee)

Comment 7

3 years ago
To verify on Desktop:

- Set up two browsers with different FxA accounts
- Select one to make an outgoing call
- Open the browser console
- Right-click in the browser console and select "Log Request and Response Bodies"
- Initiate a direct outgoing call from that browser (doesn't matter if it doesn't connect)
- Examine the console, look for:

POST https://loop.services.mozilla.com/calls

(the domain may be different if you're not on production)

- Click on that message, the Inspect Network Request window opens

Expected Results

=> The request body should have a "channel" option which is set to the update channel of the Firefox you're on, e.g. "nightly", "aurora" etc. If you're on a dev build, this should be "default", unless you've explicitly modified it.


To verify on standalone:

- Get a call url & load it in the browser
- Open the web console
- Right-click in the web console and select "Log Request and Response Bodies"
- Initiate a call from that page (doesn't matter if it doesn't connect)
- Examine the console, look for:

POST https://loop.services.mozilla.com/calls/<token>

(the domain may be different if you're not on production)

- Click on that message, the Inspect Network Request window opens


Expected Results

=> The request body should have a "channel" option with value "standalone"
https://hg.mozilla.org/mozilla-central/rev/e2f7534c7687
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8510492 [details] [diff] [review]
Add channel to POST calls for Loop to allow different servers based on the channel.

Approval Request Comment
[Feature/regressing bug #]: send channel message to external servers
[User impact if declined]: Our partner's servers could not distinguish Nightly/Aurora users from Beta/Release users which would make partner testing virtually impossible
[Describe test coverage new/current, TBPL]: unit tests, manual testing/verification
[Risks and why]: No risk outside of Hello; minimal risk to Hello -- our partner needs this message in order to do proper testing prior to SDK deployment
[String/UUID change made/needed]: No strings
Attachment #8510492 - Flags: approval-mozilla-beta?
Attachment #8510492 - Flags: approval-mozilla-aurora?
[Tracking Requested - why for this release]:
Our partner's servers need to know if a call is coming from a Nightly/Aurora user or a Beta/Release user in order to do testing
tracking-firefox34: --- → ?
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → fixed
tracking-firefox34: ? → +
Tried on Nightly 10/25 (Linux) and works (Once I got it to enable the request bodies)
I have a couple of questions about this change before approving for uplift.

Is this change really necessary? Shouldn't the partner be able to identify the channel based on the version #?

Is this expected to be a short lived addition to the packet for initial testing or is this needed long term?
Flags: needinfo?(mreavy)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #12)
> I have a couple of questions about this change before approving for uplift.
> 
> Is this change really necessary? Shouldn't the partner be able to identify
> the channel based on the version #?

My apologies that it isn't captured here, but see bug 1033032 comments 1 and 3 by abr.  The UA version doesn't tell us which API key it should be using (see https://bugzilla.mozilla.org/show_bug.cgi?id=1033032#c3 for details).  Basically, it's set up to use one API key for Nightly/Aurora, and another for Beta and Release builds, so the UA version doesn't tell you whether it's 35 Aurora or 35 Beta, which would use different keys.

> Is this expected to be a short lived addition to the packet for initial
> testing or is this needed long term?

This is a long-term addition to help our partner manage handling of backward-compatibility and to route requests to servers compatible with the protocols used in different Firefox versions. This provides a solution so they can move forward with features in the servers.
Flags: needinfo?(mreavy)
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #13)
> My apologies that it isn't captured here, but see bug 1033032 comments 1 and
> 3 by abr.  The UA version doesn't tell us which API key it should be using
> (see https://bugzilla.mozilla.org/show_bug.cgi?id=1033032#c3 for details). 
> Basically, it's set up to use one API key for Nightly/Aurora, and another
> for Beta and Release builds, so the UA version doesn't tell you whether it's
> 35 Aurora or 35 Beta, which would use different keys.

Why can't the API key be set with the version specific preferences/config that we use for other features? If the API key determines the server (or server version) with which the client should communicate, why does the channel name need to be passed to the server? It seems to me that the client should inform the server which version of the API it wants to use. The server should not care at all about client specifics like version/channel.

If we really do need to proceed in this way for some reason, it is also possible to determine the channel using the version plus calendar date. We keep the version info up-to-date on the wiki and the channel can be scraped from there instead of having the client pass this information.
Flags: needinfo?(mreavy)
Comment on attachment 8510492 [details] [diff] [review]
Add channel to POST calls for Loop to allow different servers based on the channel.

I spoke with Maire. She'll add some further details about why this change is required as is as least for the short term. Long term I would like to see a solution that doesn't use the channel names as I don't like the idea of including them in an API between Firefox and TokBox. Aurora+

Plan is to test on Aurora on Sunday and get this change into Beta4.
Attachment #8510492 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/9cf8a756d9a6
Blocks: 1089354
In the short term, we need to unblock our partner so they can roll out server features without breaking older clients, and also test the segregation procedures before actually doing the rollouts.  This change matches the agreed-on design our partner has been coding to. 

For background:  (Note - I'm dredging my memory, since the reasoning wasn't captured in this bug or the one it refers to - which is a large part of why the questions came up now.)  IIRC, one of the reasons the solution uses channels instead of SDK versions is so that there's a consistent "If I'm on Nightly/NN, I can call people using X" where I believe X is up through current release at the time Nightly/NN is built (where NN is 35, 36, etc), but I may not be able to call people on Release+1. I know we talked about several possible compatibility matrices, but those never ended up in any of the bugs and aren't on any of the wiki documentation pages that we've built thus far.  If that's incorrect and there is a wiki page about this, please reference it here.

Lawrence has expressed real concern about using "nightly/aurora/beta/release" (especially aurora) because longer term, channel name meanings may change (many proposals to change what aurora is are on the table, for example) -- even the textual names themselves may change.  They also (at best) only indirectly identify compatibility issues. 

I'm opening a new bug (Bug 1089354) to discuss changing "nightly", "aurora", "beta", etc to something we can lock down (and translate "nightly" etc to those new values).  We also should document the design, and consider if there's a better way to achieve the goals, perhaps by simply using the version number (in theory changes should be landing in a version, and once out of Nightly the API to the server should remain fairly stable (for that version) after the current spurt of uplifts).
Flags: needinfo?(mreavy)
Verified on Aurora Nightly 10/26 Linux
Comment on attachment 8510492 [details] [diff] [review]
Add channel to POST calls for Loop to allow different servers based on the channel.

># HG changeset patch
># User Adam Roach [:abr] <adam@nostrum.com>
>
>Bug 1033579 - Add channel to POST calls for Loop to allow different servers based on the channel.
>
>diff --git a/browser/components/loop/content/js/client.js b/browser/components/loop/content/js/client.js
>index c70c038..a518216 100644
>--- a/browser/components/loop/content/js/client.js
>+++ b/browser/components/loop/content/js/client.js
>@@ -227,17 +227,19 @@ loop.Client = (function($) {
>      * @param {Function} cb Callback(err, result)
>      */
>     setupOutgoingCall: function(calleeIds, callType, cb) {
>       // For direct calls, we only ever use the logged-in session. Direct
>       // calls by guests aren't valid.
>       this.mozLoop.hawkRequest(this.mozLoop.LOOP_SESSION_TYPE.FXA,
>         "/calls", "POST", {
>           calleeId: calleeIds,
>-          callType: callType
>+          callType: callType,
>+          channel: this.mozLoop.appVersionInfo ?
>+                   this.mozLoop.appVersionInfo.channel : "unknown"
>         },
>         function (err, responseText) {
>           if (err) {
>             this._failureHandler(cb, err);
>             return;
>           }
> 
>           try {
>diff --git a/browser/components/loop/standalone/content/js/standaloneClient.js b/browser/components/loop/standalone/content/js/standaloneClient.js
>index cd4df3e..7b5569a 100644
>--- a/browser/components/loop/standalone/content/js/standaloneClient.js
>+++ b/browser/components/loop/standalone/content/js/standaloneClient.js
>@@ -110,17 +110,17 @@ loop.StandaloneClient = (function($) {
>         throw new Error("missing required parameter loopToken");
>       }
> 
>       var req = $.ajax({
>         url:         this.settings.baseServerUrl + "/calls/" + loopToken,
>         method:      "POST",
>         contentType: "application/json",
>         dataType:    "json",
>-        data: JSON.stringify({callType: callType})
>+        data: JSON.stringify({callType: callType, channel: "standalone"})
>       });
> 
>       req.done(function(sessionData) {
>         try {
>           cb(null, this._validate(sessionData, expectedCallsProperties));
>         } catch (err) {
>           console.error("Error requesting call info", err.message);
>           cb(err);
>diff --git a/browser/components/loop/test/desktop-local/client_test.js b/browser/components/loop/test/desktop-local/client_test.js
>index cd91064..e09eeb2 100644
>--- a/browser/components/loop/test/desktop-local/client_test.js
>+++ b/browser/components/loop/test/desktop-local/client_test.js
>@@ -267,17 +267,33 @@ describe("loop.Client", function() {
>       it("should make a POST call to /calls", function() {
>         client.setupOutgoingCall(calleeIds, callType);
> 
>         sinon.assert.calledOnce(hawkRequestStub);
>         sinon.assert.calledWith(hawkRequestStub,
>           mozLoop.LOOP_SESSION_TYPE.FXA,
>           "/calls",
>           "POST",
>-          { calleeId: calleeIds, callType: callType }
>+          { calleeId: calleeIds, callType: callType, channel: "unknown" }
>+        );
>+      });
>+
>+      it("should include the channel when defined", function() {
>+        mozLoop.appVersionInfo = {
>+          channel: "beta"
>+        };
>+
>+        client.setupOutgoingCall(calleeIds, callType);
>+
>+        sinon.assert.calledOnce(hawkRequestStub);
>+        sinon.assert.calledWith(hawkRequestStub,
>+          mozLoop.LOOP_SESSION_TYPE.FXA,
>+          "/calls",
>+          "POST",
>+          { calleeId: calleeIds, callType: callType, channel: "beta" }
>         );
>       });
> 
>       it("should call the callback if the request is successful", function() {
>         var requestData = {
>           apiKey: "fake",
>           callId: "fakeCall",
>           progressURL: "fakeurl",
>diff --git a/browser/components/loop/test/standalone/standalone_client_test.js b/browser/components/loop/test/standalone/standalone_client_test.js
>index 843c46c..013d694 100644
>--- a/browser/components/loop/test/standalone/standalone_client_test.js
>+++ b/browser/components/loop/test/standalone/standalone_client_test.js
>@@ -123,17 +123,17 @@ describe("loop.StandaloneClient", function() {
>         });
> 
>       it("should post data for the given call", function() {
>         client.requestCallInfo("fake", "audio", callback);
> 
>         expect(requests).to.have.length.of(1);
>         expect(requests[0].url).to.be.equal("http://fake.api/calls/fake");
>         expect(requests[0].method).to.be.equal("POST");
>-        expect(requests[0].requestBody).to.be.equal('{"callType":"audio"}');
>+        expect(requests[0].requestBody).to.be.equal('{"callType":"audio","channel":"standalone"}');
>       });
> 
>       it("should receive call data for the given call", function() {
>         client.requestCallInfo("fake", "audio-video", callback);
> 
>         var sessionData = {
>           sessionId: "one",
>           sessionToken: "two",
Attachment #8510492 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox35: affected → verified
status-firefox36: fixed → verified
status-firefox33: --- → unaffected
https://hg.mozilla.org/releases/mozilla-beta/rev/d43a7b8995a6
status-firefox34: affected → fixed
Depends on: 1090103
No longer depends on: 1090103
Paul, please verify this is fixed in the next Beta build.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(paul.silaghi)
QA Contact: paul.silaghi
Verified fixed FF 34b5 on Win 7, OS X 10.9.5 using the STR in comment 7
status-firefox34: fixed → verified
Flags: needinfo?(paul.silaghi)
You need to log in before you can comment on or make changes to this bug.