1033575 - Loop server needs to select API key based on channel of client

Status: RESOLVED FIXED

(Hello (Loop) :: Server, defect, P2)

Description

[Adam Roach [:abr]]

Server needs to ingest new "channel" field in call setup messages, and select appropriate API key in response. See Bug 1033032 for details.

Comment 1

[Adam Roach [:abr]]

Based on the conversations we've had so far with TokBox, I think we're going to want to break down the channels as follows:

 nightly, aurora: Use the new Nightly/Aurora key (Bug 1033574)
 beta, release, esr, default: Use the existing deployment key (Bug 1021955)

We don't really need to worry about having this in place in the server until we're to the point of uplifting to Beta.

I suspect that once we actually get into ESR, there's a good chance that we'll be asked to use yet a third API key for that channel. It may save some work in the future if we make sure the implementation can be easily expanded to allow this use case; e.g., by allowing arbitrary channel-to-key mappings.

Comment 2

[Adam Roach [:abr]]

Rob: which key do you want us to use for calls initiated by the standalone ("link-clicker") client?

Comment 3

[Rob Hainer]

Let me ask our Product team what they think.

Comment 4

[Romain Testard [:RT]]

Can you please also confirm for the FFOS client?
I copy Fernando on the ticket for awareness.

Comment 5

[Alexis Metaireau (:alexis)]

I also need to know if the requests will be done to the same tokbox host or to a different one?

Comment 6

[Alexis Metaireau (:alexis)]

As per Adam comment ("We don't really need to worry about having this in place in the server until we're to the point of uplifting to Beta."), I'm changing the priority.

Comment 7

[Tarek Ziadé (:tarek)]

(In reply to Alexis Metaireau (:alexis) from comment #6)
> As per Adam comment ("We don't really need to worry about having this in
> place in the server until we're to the point of uplifting to Beta."), I'm
> changing the priority.

I think there are two different things here: in what dev cycle we want to add it, and within that cycle, what is the importance and priority. I'll send an e-mail to propose markers for the server cycles

Comment 8

[Rémy Hubscher (:natim)]

I think what we want is clients to send the server two Header.

The client version:

    Loop-Client-Version: 1.2.0
    Loop-Client-Channel: Nightly

We'd like the client to add this to each calls so that we can choose the API version wrt to it as well as the TokBox channel.

Comment 9

[Rémy Hubscher (:natim)]

Does this sounds good to you :abr?

Comment 10

[Alexis Metaireau (:alexis)]

Ah, this is defined in the wiki, it should be like that:

 channel: The release channel of the calling client; this is the value from the app.update.channel pref, and can be any one of:

    release
    esr
    beta
    aurora
    nightly
    default -- I believe this value indicates that the browser is not configured for automatic updates.

Comment 11

[Rémy Hubscher (:natim)]

For me the channel is not a configuration of the call but of the client.
I'd rather have this information on all call to the server.

This could be useful for other things like:
 - API version choice call_url vs callUrl as well as 
 - Stats about the client usage.

Since what we want is to configure the server wrt the client version and channel, I would rather add this as a header on every client call.

Comment 12

[Rémy Hubscher (:natim)]

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1025792#c13

Comment 13

[Rob Hainer]

Does the standalone client also have the concept of version and channel? If so, it should follow the same logic as browser clients for selecting which OpenTok apiKey to use. All requests should go to the same host. Tokbox will differentiate based on apiKey and not URL. The best solution for the FFOS client is still be discussed internally here at Tokbox.

Comment 14

[Rob Hainer]

We'd like to follow the same logic for the FFOS client as we do for all other clients. 

OpenTok apiKey maps to Tokbox server environment regardless of client. The way Tokbox sees things, the Mozilla server should decide which Tokbox environment to use for hosting the session and then communicate that decision via the apiKey. All clients that are running code based on the Nightly or Aurora code base should use the apiKey that maps to the Nighly/Aurora Tokbox server environment and all clients based on the Beta or Stable code base should use the apiKey that maps to the Beta/Stable Tokbox server environment.

Comment 15

[Daniel Coloma:dcoloma]

With respect to the FirefoxOS client, there is no similar concept to the release channel used in Firefox Desktop. 

The information we could send to Loop server to let it distinguish which apiKey to use is:

- Although the FFOS version is not available to privileged apps (such as Loop), what is available is the UserAgent Header and based on that we could determine FFOS version [1] or send the Gecko version.
- The Loop client version

Rob, Adam, any ideas about how to move this forward?

[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Gecko_user_agent_string_reference#Firefox_OS

Comment 16

[Rémy Hubscher (:natim)]

Yes Robert, it is what we plan to do on server side.
Daniel, I think your channels are a bit different for FFxOS.
I would rather base the channel selection on the Loop Client version.

Can you tell if it is a beta or release from the User-Agent?

Comment 17

[Daniel Coloma:dcoloma]

(In reply to Rémy Hubscher (:natim) from comment #16)
> Yes Robert, it is what we plan to do on server side.
> Daniel, I think your channels are a bit different for FFxOS.
> I would rather base the channel selection on the Loop Client version.
> 
> Can you tell if it is a beta or release from the User-Agent?

Not really, for instance as of today there are multiple FFOS release versions commercialized: 1.0, 1.1 and 1.3. All of them are official releases and using different gecko versions.

Comment 18

[Rémy Hubscher (:natim)]

Attachment: Link to github PR

Comment 19

[Rob Hainer]

Sending all FFOS traffic to the Tokbox Beta/Stable environment is not necessarily a bad approach. Is there even a known use case where there will be a FFOS client based on a Nightly code base that is publicly available? Having some flexibility with regard to which Tokbox environments hosts the sessions is a good idea but it may make sense to revisit the business requirements and use cases to determine the appropriate course of action.

Comment 20

[Adam Roach [:abr]]

Comment on attachment 8453125 [details]
Link to github PR

This looks functionally good. I will note that the server URL won't change based on the channel, at least for the foreseeable future, but there's no harm having code in here to allow it to be configurable on a per-channel basis.

I do note that the tokbox.js code uses self._opentok in the callback (twice), which is going to fall back to general (non-channel-specific) instance. That's not what you want.

I also have some concerns about the garbage collection implications of constructing a new OpenTok instance each time a channel-associated request comes in (these may be unfounded -- how heavy are these objects?). 

If they're large or expensive to construct, we could avoid some of the GC churn by caching these objects, keyed on apiKey; in other words, rather than:

    if (this.credentials.hasOwnProperty(options.channel)) {
      opentok = new exports.OpenTok(...);
    } else {
      opentok = this._opentok;
    }

You could do something like this (adding in serverURL if you like):

  var apiKey = this.apiKey;
  var apiSecret = this.apiSecret;

  if (this.credentials.hasOwnProperty(options.channel)) {
    apiKey = this.credentials[options.channel].apiKey;
    apiSecret = this.credentials[options.channel].apiSecret;
  }

  if (!this._opentok[apiKey]) {
    this._opentok[apiKey] = new exports.OpenTok(apiKey, apiSecret);
  }
  opentok = this._opentok[apiKey];

(This also has the property of not having a naked this._opentok lying around to be confused with the channel-specific instances)

Finally, and I might just be missing it, I don't see where the first new unit test in tokbox_test.js checks for the default API key and default secret.

Comment 21

[Alexis Metaireau (:alexis)]

Comment on attachment 8453125 [details]
Link to github PR

I commented on the pull requests the reasons, but we're going in the right direction, awesome :)

Comment 22

[Adam Roach [:abr]]

(In reply to Daniel Coloma:dcoloma from comment #15)
> With respect to the FirefoxOS client, there is no similar concept to the
> release channel used in Firefox Desktop. 

I think it's probably reasonable to treat the mobile application as its own channel so that we can make simple configuration changes at the server to suit business needs as they change. What would you think about setting "channel" to "mobile" for the mobile client?

Comment 23

[aoberoi]

Hey guys,

I think abr's idea of storing the OpenTok instanced in a keyed object totally makes sense.

From the POV of GC and cost of the objects, it should be marginal. Most of the functions are stored on the shared prototype (and prototypes of the dependent objects). Theres < 10 small functions in the instance itself and < 10 small string properties. If there's a more precise way to measure the size of the instances, I'm not aware of it.

Comment 24

[Daniel Coloma:dcoloma]

(In reply to Adam Roach [:abr] from comment #22)
> (In reply to Daniel Coloma:dcoloma from comment #15)
> > With respect to the FirefoxOS client, there is no similar concept to the
> > release channel used in Firefox Desktop. 
> 
> I think it's probably reasonable to treat the mobile application as its own
> channel so that we can make simple configuration changes at the server to
> suit business needs as they change. What would you think about setting
> "channel" to "mobile" for the mobile client?

That makes sense to me, what about Product Team?

Comment 25

[Daniel Coloma:dcoloma]

(In reply to Rob Hainer from comment #19)
> Is there even a known use case where there will
> be a FFOS client based on a Nightly code base that is publicly available?

Dogfooders, early/betat testers or hardcore lovers :) might be using devices with FFOS releases not officially relased yet, but this is a marginal number that I don't think should be relevant. I like Adam suggestion to use "mobile" for FFOS Loop for the time being.

Comment 26

[Romain Testard [:RT]]

OK so if I summarize this (and if I don't get it wrong...), what we are saying is:

 nightly, aurora: Use the new Nightly/Aurora key 
 beta, release, esr, default: Use the existing deployment key 
 FFOS: Use the "mobile" key

The key used will be the one set by the calling party's platform and will define the Tokbox server environment to use. Although the TokBox server environments will be running different code releases and may support different features or not.

Not sure about the complexity involved here but it seems each client could be having to manage feature inter-operability with potentially 3 types of server environments, which I assume all will have to be tested.

Am-I understanding this right?

Comment 27

[Adam Roach [:abr]]

(In reply to Romain Testard [:RT] from comment #26)
> OK so if I summarize this (and if I don't get it wrong...), what we are
> saying is:
> 
>  nightly, aurora: Use the new Nightly/Aurora key 
>  beta, release, esr, default: Use the existing deployment key 
>  FFOS: Use the "mobile" key
> 
> The key used will be the one set by the calling party's platform and will
> define the Tokbox server environment to use. Although the TokBox server
> environments will be running different code releases and may support
> different features or not.
> 
> Not sure about the complexity involved here but it seems each client could
> be having to manage feature inter-operability with potentially 3 types of
> server environments, which I assume all will have to be tested.
> 
> Am-I understanding this right?

For now, TokBox wants us to use the same API key for Mobile as we do for Release. I'm only proposing a new channel identifier for mobile so that we have the ability to change this in the future, if it becomes necessary to do so.

So, for now, we can assume only two TokBox server environments.

Comment 28

[Rob Hainer]

I concur with ABR's proposal of maintaining flexibility with regard to mapping channels to API keys. For now Tokbox is planning two and only two Tokbox server environments that will serve Mozilla client traffic. It would be nice to be able to revisit that in the future without creating too much Mozilla server work.

Comment 29

[Romain Testard [:RT]]

OK, makes sense, thanks for clarifying.

Comment 30

[Rémy Hubscher (:natim)]

https://github.com/mozilla-services/loop-server/commit/25d220e641273b6089b2d977fe9e2e654ff15cde

Comment 31

[Chris Karlof [:ckarlof]]

I'm a little late to this party, but I'm concerned about using the release channel as a proxy for client capabilities/intentions. This has a couple problems:

1) What about clients that aren't on a Fx release channel, e.g., future iOS, Android clients, and Web clients on Chrome? This has already come up here in this thread: https://bugzilla.mozilla.org/show_bug.cgi?id=1033575#c15 for FxOS. I don't see "keep adding channels" as a maintainable solution over time, especially as clients evolve. 

2) Going forward, how does the server distinguish between future and legacy "release" channel clients that may support newer (or older versions) of the TokBox API or have different capabilities? E.g., Fx 34 release may use TokBox v2, but Fx 35 release may use TokBox v3. 

I feel the heart of the matter here is the actual client capabilities, not the release channel per se. I suggest we signal capabilities directly in terms of the TokBox API version(s) and other things the client supports. E.g., capabilities could be provided to the server like:

capabilities: {
  tokbox: "v2",
  webrtc: "1.0",
}

This is similar to "user agent sniffing" vs "feature detection" in client side web dev. Feature detection is strongly preferred due the ambiguity and futility in scaling in the former (there are now hundreds of user agents).

Comment 32

[Alexis Metaireau (:alexis)]

I believe this need came up because we have different stability needs for the different clients.

For firefox, this translates to having a different release channel, but that's true that the need is not tied to the release channels.

We could have the client send the stability needs it has in the request and we could act and decide which API key to use from there for instance.

I'm adding Adam in needinfo here since he's the API designer and he came up with this decision at first. Adam, would you mind adding thoughts here about the proposals we're making?

To answer point by point to your questions, Chris:

1) Clients will not need to send this information. A default is configured on the server and will be used in case no release channel is specified.

2) As I said, I don't think the thing we're detecting is the webrtc version or the tokbox version here, but more the stability the client needs. The tokbox server is used by loop-server to retrieve tokens that will be used by the client to connect to their SDK (which will connect together clients).

That being said, having intents sent by the client is something that makes sense, and I would prefer having the clients sending a "stability" indicator rather than a release channel.

Comment 33

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #32)
> 2) As I said, I don't think the thing we're detecting is the webrtc version
> or the tokbox version here, but more the stability the client needs. The
> tokbox server is used by loop-server to retrieve tokens that will be used by
> the client to connect to their SDK (which will connect together clients).

Yes, this is the salient point in the conversation. We're not pegging to capabilities; we're indicating the level of stability required for the server -- basically, our Nightly/Aurora clients will be connecting to TokBox's beta server deployments. They don't want production clients on these servers, since they may not be sufficiently stable for actual production use.

There may be some assumptions made on the TokBox servers about likely capabilities, but these are secondary to the stability aspect.

> That being said, having intents sent by the client is something that makes
> sense, and I would prefer having the clients sending a "stability" indicator
> rather than a release channel.

In the conversations so far with TokBox, one item that keeps coming up is that we need flexibility here to be able to move things around. If we (Mozilla and TokBox in consultation) decide that purposes are better served by putting Firefox Beta users on the pre-production version of TokBox's servers, we should be able to do that without having to push out a new version of Beta. Similarly, if we have a good reason to move the mobile users onto a different set of servers, that should be possible without updating the Mobile Loop/Hello client.

So, I'm happy to cut this along other lines, but what we really want here is broad classes of clients, and we want to treat the desktop channels as their own classes.

Comment 34

[Chris Karlof [:ckarlof]]

I'm less concerned if the "release channel" signal is being used for the purposes described above.

It still seems a bit odd to me that the Loop Server is making a decision about which Tokbox deployment to route the client to, but doesn't have explicit information that the client supports the API version offered by the Tokbox server.

If Tokbox ever wants to change/version their API in non-backward compatible way, I suspect this issue will come up. It would also come up if Tokbox wants to route capable clients to a beta deployment of new API. Currently, it seems that we would have to resort to UA/Channel sniffing, which I wouldn't be a fan of.

Comment 35

[James Bonacci [:jbonacci]]

There are four meaty comments after the status change of this bug.
Are we OK to Verify this bug now?

Comment 36

[Rémy Hubscher (:natim)]

Commit https://github.com/mozilla-services/loop-server/commit/25d220e641273b6089b2d977fe9e2e654ff15cde has been merged so yes you can select the right TokBox credentials based on the release channel but that's it for now.

Also I don't know if clients are using it already for Beta.