Add Callstats api calls #261

Merged
merged 18 commits into from Jul 6, 2016

Projects

None yet

5 participants

@KaptenJansson
Member

Fixes #214

@googlebot googlebot added the cla: yes label Feb 5, 2016
@KaptenJansson KaptenJansson Add Callstats api calls
e36574a
@karthikbr82 karthikbr82 and 2 others commented on an outdated diff Feb 5, 2016
src/web_app/js/peerconnectionclient.js
+ if (typeof $ !== 'function' && typeof io !== 'function' &&
+ typeof jsSHA !== 'function') {
+ trace('Callstats dependencies missing, stats will not be setup.');
+ return;
+ }
+
+ trace('Setting up callstats.');
+ // jscs:disable requireCapitalizedConstructors
+ /* jshint newcap: false */
+ var callStats = new callstats($, io, jsSHA);
+ // jscs:enable requireCapitalizedConstructors
+ /* jshint newcap: true */
+
+ var appId = '423310139';
+ var appSecret = 'jpZu5pquoROlCn5gekQX1RzTHPk=';
+ var userId = this.params_.clientId;
@karthikbr82
karthikbr82 Feb 5, 2016

We recommend not committing the appID and appSecret on github. And setting the originURL on callstats.io's appID settings.

@KaptenJansson
KaptenJansson Feb 5, 2016 Member

I've set the originURL in the callstats dashboard setting ofc. Regarding the secrets, does it matter as long as we have the originURL restriction in place?

@KaptenJansson
KaptenJansson Feb 5, 2016 Member

But I could add the secrets as part of this.params_ which in turn could get them from a GAE secure variable/store.

@juberti
juberti Feb 5, 2016 Contributor

yes, please be sure to read the secrets from a build-time config or some other non-public location. People will still be able to snoop the secrets via view-source, but this way they won't get reused when apprtc gets cloned.

@KaptenJansson KaptenJansson and 3 others commented on an outdated diff Feb 5, 2016
src/web_app/html/index_template.html
@@ -28,6 +28,11 @@
<link rel="canonical" href="{{ room_link }}">
{% endif %}
<link rel="stylesheet" href="/css/main.css">
+ <!-- callstats dependencies: http://www.callstats.io/api/#integrating-with-your-sdk -->
+ <script src="https://api.callstats.io/static/callstats.min.js"></script>
+ <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
+ <script src="https://cdn.socket.io/socket.io-1.2.0.js"></script>
+ <script src="https://cdnjs.cloudflare.com/ajax/libs/jsSHA/1.5.0/sha.js"></script>
@KaptenJansson
KaptenJansson Feb 5, 2016 Member

I'm thinking we should perhaps pull down the libs we can and commit them in src/third_party.

@juberti
juberti Feb 5, 2016 Contributor

This seems like a lot of lib code that we would be adding. Is this all really needed?

@juberti
juberti Feb 5, 2016 Contributor

@karthikbr82 @vr000m Callstats.js is 19K. AppRTC's JS is 20K.

The other supporting JS is 56K. What does callstats need from jquery and socket.io?

@vr000m
vr000m Feb 5, 2016

Socket.io is the transport for sending stats. Jquery, perhaps could be deprecated but I'll let @karthikbr82 or @marcinag answer that.

@karthikbr82
karthikbr82 Feb 8, 2016

jQuery will be deprecated in the version 3.10.x, which will be released on Tuesday.

@KaptenJansson
KaptenJansson Feb 8, 2016 Member

Does that mean we can just omit the $ function/object and the jquery lib? I guess your API documentation will be updated to reflect this.

@vr000m
vr000m Feb 8, 2016

Yes, the first argument of the initialize() would then need to be null for backwards compatibility reasons. We will update the documentation and send you a pointer to it.

If we get webcrypto tested on all browsers, we could also do away with the jsSHA reference as well.

@juberti
juberti Feb 8, 2016 Contributor

Thanks for working on this. Would it be possible to use straight websockets instead of socket.io?

@karthikbr82
karthikbr82 Feb 9, 2016

callstats.js version 3.10.0 is available now, it deprecates the jquery usage, so the $ as first argument can be removed.

var callStats = new callstats(null,io,jsSHA);

We also added two new fabric events, screenSharingStart and screenSharingStop to track the screen sharing start/stop events during the call.

@juberti
juberti Feb 9, 2016 Contributor

Thanks. I think it's worth considering hosting the JS libs as GAE static content. While I'm not sure this will be faster, it will avoid DNS lookups and new connections, so I think there's a good chance it will be better.

Longer-term we can try to optimize away SHA and possibly socket.

@vr000m
vr000m Feb 9, 2016

This is on our roadmap, will send an update when this would be available.

@KaptenJansson
KaptenJansson Feb 11, 2016 Member

jssha and socket-io are now downloaded via NPM and included in the closure compiler step. callstats.io is the only external/dynamic resource.

We could download it in the build step using CURL and then include when compiling as well? Then ofc we need to push a new version of apprtc if we want new callstats.io changes.

@juberti juberti and 2 others commented on an outdated diff Feb 5, 2016
src/web_app/js/util.js
/* globals chrome */
'use strict';
-
-function $(selector) {
+// Had to rename this due to conflicting with jquery.
+// TODO: Rename?
+function $1(selector) {
@juberti
juberti Feb 5, 2016 Contributor

This is unfortunate...

@vr000m
vr000m Feb 10, 2016

perhaps this change can be rolled back.

@KaptenJansson
KaptenJansson Feb 11, 2016 Member

I've reverted this.

@KaptenJansson KaptenJansson address comments
4d812ec
@KaptenJansson
Member

Now you can set the callstats properties in local environment variables prior building AppRTC and it will pick these upp and add them to a file in the out folder which is then picked up when creating the HTML template and they are added to the page along with the other params:

export APPID='7777777' APPSECRET='HFDUIAUIHA//&%%¤4566773'
grunt build

If they are not found, the value 'None' will be used instead.

KaptenJansson added some commits Feb 11, 2016
@KaptenJansson KaptenJansson Remove jquery, revert $ name change, add shajs and socket-io as stati…
…c libs via npm
8c9b876
@KaptenJansson KaptenJansson remove redundant comment
3a7841f
@KaptenJansson
Member

@karthikbr82 Out of curiosity, is there are reason the callstats constructor does not start with a capitalized C?

Also, do any one of you see anything more that's needed for an initial implementation or can we merge this?

@juberti juberti and 1 other commented on an outdated diff Feb 12, 2016
src/web_app/js/peerconnectionclient.js
@@ -38,6 +38,7 @@ var PeerConnectionClient = function(params, startTime) {
this.pc_.onsignalingstatechange = this.onSignalingStateChanged_.bind(this);
this.pc_.oniceconnectionstatechange =
this.onIceConnectionStateChanged_.bind(this);
+ this.pc_.onnegotiationneeded = this.setupCallStats_();
@juberti
juberti Feb 12, 2016 Contributor

why is this done on onnegotiationneeded?

@KaptenJansson
KaptenJansson Feb 12, 2016 Member

It's the earliest it can be attached to the peerConnection according the callstats api documentation. Since we did not use that event for anything I thought we might as well use it.

@juberti juberti and 1 other commented on an outdated diff Feb 12, 2016
src/web_app/js/peerconnectionclient.js
+ var callback = function(status, msg) {
+ trace('Callstats status: ' + status + ' msg: ' + msg);
+ };
+
+ // Init the callstats api.
+ callStats.initialize(appId, appSecret, userId, callback, statsCallback,
+ configParams);
+
+ // Hookup the callstats api to the peerconnection object.
+ callStats.addNewFabric(this.pc_, remoteUserId, usage,
+ conferenceId, callback);
+
+ this.callStats = callStats;
+ this.conferenceId = this.params_.roomId;
+ } catch (error) {
+ trace('Callstats could not be setup: ' + error);
@juberti
juberti Feb 12, 2016 Contributor

setup -> set up

@juberti
Contributor
juberti commented Feb 12, 2016

Let's make sure we have a thumbs-up from legal on the privacy policy stuff before we commit and/or push this.

@karthikbr82 karthikbr82 and 3 others commented on an outdated diff Feb 12, 2016
src/web_app/js/peerconnectionclient.js
+ // http://www.callstats.io/api/#enumeration-of-fabricusage
+ // TODO: Might need to change this dynamically if an audio/video only call.
+ var usage = callStats.fabricUsage.multiplex;
+ var statsCallback = null;
+ var configParams = null;
+ var callback = function(status, msg) {
+ trace('Callstats status: ' + status + ' msg: ' + msg);
+ };
+
+ // Init the callstats api.
+ callStats.initialize(appId, appSecret, userId, callback, statsCallback,
+ configParams);
+
+ // Hookup the callstats api to the peerconnection object.
+ callStats.addNewFabric(this.pc_, remoteUserId, usage,
+ conferenceId, callback);
@karthikbr82
karthikbr82 Feb 12, 2016

Why is the remoteuserId passed as NA? It should pass the actual remoteUserId because it has dependency in dashboard to view the connection between each participant pairs. If its unavailable at this point, addNewFabric can be called before createOffer/createAnswer

@KaptenJansson
KaptenJansson Feb 12, 2016 Member

We do not pass the remote clientID's to the clients hence it only knows it's own ID. Since we pass in the conferenceId and only do 2way calls it's not a big deal.

@KaptenJansson
KaptenJansson Feb 12, 2016 Member

But we could perhaps look into this if it's easy to fix.

@juberti
juberti Feb 15, 2016 Contributor

I wonder if we could simply generate a pseudo-id based on the conference name. E.g. caller id becomes "-0", callee id is "-1". Then that would be the same on both sides and could be used for correlation.

@vr000m
vr000m Feb 15, 2016

there are perhaps two solutions:

  1. setting up callstats just before createOffer may solve the issue of the
    calls where the peer connection is created but callee did not join. This
    will of course mess with our setup time, gathering time calculations for
    the app. :(
  2. An alternate solution, would be to setup callstats before onNN (like it
    is now). However, explicitly call the fabricTerminated if the tab/call is
    closed without the caller joining.

cc: @karthikbr82

On Tue, 16 Feb 2016 at 01:07, Justin Uberti notifications@github.com
wrote:

In src/web_app/js/peerconnectionclient.js
#261 (comment):

  • // http://www.callstats.io/api/#enumeration-of-fabricusage
  • // TODO: Might need to change this dynamically if an audio/video only call.
  • var usage = callStats.fabricUsage.multiplex;
  • var statsCallback = null;
  • var configParams = null;
  • var callback = function(status, msg) {
  •  trace('Callstats status: ' + status + ' msg: ' + msg);
    
  • };
  • // Init the callstats api.
  • callStats.initialize(appId, appSecret, userId, callback, statsCallback,
  •  configParams);
    
  • // Hookup the callstats api to the peerconnection object.
  • callStats.addNewFabric(this.pc_, remoteUserId, usage,
  •  conferenceId, callback);
    

I wonder if we could simply generate a pseudo-id based on the conference
name. E.g. caller id becomes "-0", callee id is "-1". Then that would be
the same on both sides and could be used for correlation.


Reply to this email directly or view it on GitHub
https://github.com/webrtc/apprtc/pull/261/files#r52951984.

@juberti
juberti Feb 16, 2016 Contributor

calling fabricTerminated seems reasonable. What would be the downside of that approach?

@KaptenJansson
KaptenJansson Feb 16, 2016 Member

I think the solution proposed by @juberti is best since the clientId's generated are random and cannot be used for correlation (unless stored somewhere) hence it's not worth passing it to the other side.

@KaptenJansson
KaptenJansson Feb 16, 2016 Member

Regarding solution 2, callstats is setup onNN, not before.

@karthikbr82

@KaptenJansson We know the capitalization issue. It will be capitalized in 4.0.0 version. There is one optional API associateMstWithUserID. It would be great if appRTC integrates this as well.

@vr000m
vr000m commented Feb 12, 2016

@KaptenJansson I believe we made the transition from Capital Callstats to small callstats, to match up with general AMD naming (since we supported require.js). I believe browserify does not have this naming restriction. And perhaps in v4.0, we will prolly fix this.

@KaptenJansson
Member

Re constructor caps: I see, it's not a big deal since this can be bypassed for the linters (we hit the same issue for browser prefixed constructors such as webkitRTCPeerConnection etc). I was just curious ;).

@vr000m
vr000m commented Feb 16, 2016

Thanks @KaptenJansson for reporting this. We will look into this. You can send the dashboard bugs to dashboard-support@callstats.io. For example a link to the search query or on which page the error occurred would provide sufficient initial context.

cc: @satucallstats.

@KaptenJansson
Member

Done

@KaptenJansson KaptenJansson Fix copy+paste mistakes, add associateMstWithUserId API, setup callst…
…ats in startAsCaller/Callee
9381a1e
@KaptenJansson
Member

Also fixed the userId logic to be conferanceId-0 and -1 for local and remote id respectively.

@KaptenJansson
Member

(removed my bug report here due to sending it to the email provided and wanted to save some space)

@KaptenJansson
Member

Due to the callstats dashboard truncating userId after 8 characters the users cannot currently be distinguished from each other. One temp workaround is to reverse this and prefix with 0- and 1- instead.

@karthikbr82

Hi,

We are rolling out the dashboard truncating fix later today. So you may not
need the workaround.

On Wed, Feb 17, 2016 at 8:51 PM, Christoffer Jansson <
notifications@github.com> wrote:

Due to the callstats dashboard truncating userId after 8 characters the
users cannot currently be distinguished from each other. One temp
workaround is to reverse this and prefix with 0- and 1- instead.


Reply to this email directly or view it on GitHub
#261 (comment).

http://www.callstats.io
Analytics and Optimizations for WebRTC.

@KaptenJansson
Member

Awesome work ;)

@KaptenJansson
Member

Just verified the dashboard truncating fix, looks really good now!

@KaptenJansson KaptenJansson Fix some comments
4ebdfb1
@vr000m
vr000m commented Feb 18, 2016

The truncation is now set to 14, it should solve your issue.

@KaptenJansson
Member

Indeed it did. Now it displays the users separately.

KaptenJansson added some commits Feb 25, 2016
@KaptenJansson KaptenJansson fix getUserMediaError
fd7418b
@KaptenJansson KaptenJansson Merge branch 'master' into callstats
97c1b6e
@KaptenJansson KaptenJansson Add gumError reporting, fix associtateMstWithUserID, add more events …
…+ cleanup
a01a8b1
@KaptenJansson
Member

@vr000m @karthikbr82 I've now added gUM error reporting, fabricFailed and fabricTerminated events as discussed. Also I fixed the associateMstWithUserID logic, one thing though is that the usageLabel is not populated, could you check if the labels are sent in properly and maybe just not displayed on the dashboard?

@vr000m
vr000m commented Feb 28, 2016

AFAICT, the usageLabel is not present. @karthikbr82
https://github.com/karthikbr82 can confirm from the logs?

Although on Friday, I do remember seeing mslabel or similar .

We see:

  1. cname: "xx"
  2. codec:"VP9"
  3. mediaType : "video"
  4. msid:"id1 id2"
    5.

On Sun, Feb 28, 2016, 10:43 Christoffer Jansson notifications@github.com
wrote:

@vr000m https://github.com/vr000m @karthikbr82
https://github.com/karthikbr82 I've now added gUM error reporting,
fabricFailed and fabricTerminated events as discussed. Also I fixed the
associateMstWithUserID logic, one thing though is that the usageLabel is
not populated, could you check if the labels are sent in properly and maybe
just not displayed on the dashboard?


Reply to this email directly or view it on GitHub
#261 (comment).

@KaptenJansson
Member

Hmm that is strange, I'm passing in the mediaStreamTrack label for both send and receive SSRC's (send should have real device names while receive only has an ID string as explained below).

One question though, remote track labels are just an ID due to the client only providing a device label on local tracks and this is not passed on the to the other side (unless the signalling/conferance server does it). In the example http://www.callstats.io/api/#step-6-optional-associatemstwithuserid it sets the same label for incoming and outgoing SSRC's.

@vr000m
vr000m commented Feb 28, 2016

@KaptenJansson I believe we can make the association of a local SSRC's usageLabel with remote SSRC's usageLabel.

We are currently not doing this because in some cases the local SSRCs are rewritten by the middlebox and do not appear at the remote end. Perhaps a simple solution here is that if the local usageLabel exists, we show that else show the remote. (@karthikbr82 confirm if this can be done).

@vr000m
vr000m commented Feb 29, 2016

@KaptenJansson: callstats.js mainly needs the mapping of the local outbound SSRCs and the associated usageLabel.

The usageLabel for the remote SSRC is only useful in the case where the endpoint may not be monitored (this is not the case here). for e.g., legacy interop with SIP phones or similar, we then backfill the SSRCs and usageLabel from the information provided by the other participants.

@karthikbr82

@KaptenJansson associateMstWithUserID is not being called, In firefox console log i got this error TypeError: videoSendSsrcLine is null, which is the just above associateMstWithUserID call.

@KaptenJansson
Member

Right, I've not actually tested that this works in Firefox (doh) ;). Will take another look.

@KaptenJansson
Member

Re local outbound SSRC's, then I will remove the remote SSRC mapping code.

@vr000m
vr000m commented Mar 1, 2016

Hmm. We do need the usage label for the outbound SSRCs. For example, the
front or back camera, or screenshare is only known by the generating or
sending endpoint.
On Tue, 1 Mar 2016 at 12:40, Christoffer Jansson notifications@github.com
wrote:

Re local outbound SSRC's, then I will remove the outgoing SSRC mapping
code.


Reply to this email directly or view it on GitHub
#261 (comment).

@KaptenJansson
Member

Sorry I meant remote SSRC's. Updated my comment to reflect that.

@KaptenJansson
Member

I've now double checked on Chrome, I do send the usageLabel but it still does not appear in the dashboard. Could you check the data for call 492781144 and let me know what you see?

@vr000m
vr000m commented Mar 1, 2016

The remote SSRCs are useful for getting the statistics and events out from the videoTag.

Perhaps setting the usageLabels for the remote SSRCs as null?

@KaptenJansson
Member

Re firefox, it does not use SSRC (at least not for FF to FF) hence why it cannot find any in the SDP.

@KaptenJansson
Member

Ok, will leave the remote SSRC handling as it was.

Still curious to see what data reaches the dashboard AFAIK i can see it's sent.

@KaptenJansson KaptenJansson Improve associateMstToID handling, simplify API param handling
57d7ebc
@juberti juberti and 1 other commented on an outdated diff Mar 2, 2016
@@ -146,7 +146,9 @@ module.exports = function(grunt) {
files: {
// Destination: [source files]
'out/app_engine/js/apprtc.debug.js': [
- 'src/web_app/js/analytics.js',
+ 'node_modules/jssha/src/sha.js',
@juberti
juberti Mar 2, 2016 Contributor

Add a TODO to remove these when callstats has been updated

@juberti
juberti Mar 2, 2016 Contributor

Should we consider serving these from the same place as where callstats is being served from (i.e. static content)?

@KaptenJansson
KaptenJansson Mar 3, 2016 Member

So instead of adding it to the compiled output you want them to be served on GAE behind a handler and let AppRTC make a HTTP request for it? What do we gain from that in this case? (I do understand the benefit of callstats.min.js as it's hosted elsewhere and is not available via NPM).

@KaptenJansson
KaptenJansson Mar 3, 2016 Member

Will add a TODO to remove once they are no longer needed.

@KaptenJansson
KaptenJansson Mar 3, 2016 Member

Added them as static content.

@juberti juberti and 1 other commented on an outdated diff Mar 2, 2016
src/web_app/html/index_template.html
@@ -28,6 +28,8 @@
<link rel="canonical" href="{{ room_link }}">
{% endif %}
<link rel="stylesheet" href="/css/main.css">
+ <!-- callstats dependencies: http://www.callstats.io/api/#integrating-with-your-sdk -->
+ <script src="https://api.callstats.io/static/callstats.min.js"></script>
@juberti
juberti Mar 2, 2016 Contributor

Do we want to consider serving this from GAE static content?

@karthikbr82
karthikbr82 Mar 3, 2016

make sure to pull callstats.js periodically, so that the latest version of callstats.js is in use.

@juberti juberti and 1 other commented on an outdated diff Mar 2, 2016
src/web_app/js/call.js
}.bind(this))
.catch(function(e) {
- this.onError_('Create PeerConnection exception: ' + e.message);
+ this.onError_('Create PeerConnection exception: ' + e);
+ // Let the callstats backend know we failed connecting to the
+ // other endpoint.
+ this.pcClient_.sendCallstatsEvents('fabricFailed');
@juberti
juberti Mar 2, 2016 Contributor

I don't like having code duplication for this everywhere. The fail/terminate should happen in one place, e.g. hangup, or some cleanup function (perhaps pc.close()).

@juberti
juberti Mar 2, 2016 Contributor

actually, this one is complicated since it's not clear that PC client even exists at this point.

@KaptenJansson
KaptenJansson Mar 2, 2016 Member

Especially since AppRTC does not tear down anything upon failure.

@juberti
juberti Mar 2, 2016 Contributor

Seems like an opportunity to fix that...

@KaptenJansson
KaptenJansson Mar 3, 2016 Member

Yes indeed, took a quick look and it's not an easy task. Seems like it's too big for this PR.

@juberti juberti and 1 other commented on an outdated diff Mar 2, 2016
src/web_app/js/peerconnectionclient.js
+ // TODO: Might need to change this dynamically if an audio/video only call.
+ var usage = this.callstats.fabricUsage.multiplex;
+ var callback = function(status, msg) {
+ trace('Callstats status: ' + status + ' msg: ' + msg);
+ }.bind(this);
+ // Hookup the callstats api to the peerConnection object.
+ this.callstats.addNewFabric(this.pc_, this.remoteUserId, usage,
+ this.conferenceId, callback);
+ } catch (error) {
+ trace('Callstats could not be set up: ' + error);
+ }
+};
+
+// Associate send SSRC's with media stream tracks and user ID in the
+// callstats backend.
+PeerConnectionClient.prototype.bindSendMstToIdForCallstats_ = function() {
@juberti
juberti Mar 2, 2016 Contributor

This seems like it could be unified with the function below. Also, I don't understand the comment.

@KaptenJansson
KaptenJansson Mar 2, 2016 Member

Unified it and rewrote the comment.

@juberti juberti and 1 other commented on an outdated diff Mar 2, 2016
src/web_app/js/peerconnectionclient.js
+ audioReceiveTrackId);
+
+ // Firefox does not use SSRC.
+ if (audioReceiveSsrcLine !== null) {
+ var audioReceiveSsrc = audioReceiveSsrcLine[0].match('[0-9]+');
+ this.callstats.associateMstWithUserID(this.pc_, this.remoteUserId,
+ this.conferenceId, audioReceiveSsrc[0], audioReceiveTrackLabel,
+ remoteVideoTagId);
+ }
+ }
+};
+
+// Send events to callstats backend.
+PeerConnectionClient.prototype.sendCallstatsEvents = function(fabricEvent) {
+ // http://www.callstats.io/api/#enumeration-of-fabricevent
+ var fabricEvents = {
@juberti
juberti Mar 2, 2016 Contributor

why not just pass through the right events from the start, instead of translating them here? Or maybe create our own constants?

@KaptenJansson
KaptenJansson Mar 2, 2016 Member

Sure, will pass the right events.

@juberti juberti and 1 other commented on an outdated diff Mar 2, 2016
src/web_app/js/call.js
@@ -228,6 +231,9 @@ Call.prototype.onRemoteHangup = function() {
// On remote hangup this client becomes the new initiator.
this.params_.isInitiator = true;
+ // Let the callstats backend know we are closing the call.
+ this.pcClient_.sendCallstatsEvents('fabricTerminated');
@juberti
juberti Mar 2, 2016 Contributor

PC client should probably automatically send the terminated event when .close is called.

@KaptenJansson
KaptenJansson Mar 3, 2016 Member

Agree, done.

@juberti juberti and 1 other commented on an outdated diff Mar 2, 2016
src/web_app/js/peerconnectionclient.js
@@ -319,6 +322,18 @@ PeerConnectionClient.prototype.onIceConnectionStateChanged_ = function() {
if (this.pc_.iceConnectionState === 'completed') {
trace('ICE complete time: ' +
(window.performance.now() - this.startTime_).toFixed(0) + 'ms.');
+ // Associate SSRC's with media stream tracks and user ID in the
@juberti
juberti Mar 2, 2016 Contributor

is ICE connection state the right thing to key off of? Should this be on signaling state returning to stable the first time?

@KaptenJansson
KaptenJansson Mar 2, 2016 Member

Did not event think of the other states ;). We need both localDescription and remoteDescription SDP with SSRC's before we can make the call and this seems the best place (if not the only place) in peerconnectionclient.js

@KaptenJansson
KaptenJansson Mar 2, 2016 Member

Just tested this and the remoteStreams are not set when signallingState reaches stable.

@KaptenJansson
KaptenJansson Mar 2, 2016 Member

It seems onRemoteStreamAdded_ is the best place to add it actually.

KaptenJansson added some commits Mar 2, 2016
@KaptenJansson KaptenJansson Remove boilerplate code, merge bindMstToId methods, call bindMstToId …
…on onremotestreamadded

Serve callstats via GAE static content.
42056c3
@KaptenJansson KaptenJansson Merge branch 'master' into callstats
233d55f
@KaptenJansson KaptenJansson install requests module on travis
d546128
@KaptenJansson
Member

I realized I forgot to add a comment here after fixing the comments a while ago.

I've now addressed the comments, callstats and it's dependencies are served as static content on GAE.
Also cleaned up the rest according to comments.

One thing I've not done which I think is out of scope (due to the potential size/complexity) for this PR is to improve the tear down handling if a cail fails.

@KaptenJansson
Member

Will do one more check before we can merge this, looked like the mute audio and video events weren't working anymore.

@vr000m
vr000m commented Mar 21, 2016

@KaptenJansson: we added two more failures cases via reportError (by the application):

  • iceConnectionFailure: ICE connection failure detected by the application
  • signallingError: signalling related errors in the application
@KaptenJansson
Member

Awesome! Will add those as well.

@KaptenJansson
Member

I will revive this and add the latest updates next week.

KaptenJansson added some commits Apr 15, 2016
@KaptenJansson KaptenJansson Merge branch 'master' into callstats
e231af7
@KaptenJansson KaptenJansson Fix build steps + event handler
ae844e2
@KaptenJansson
Member
KaptenJansson commented Apr 18, 2016 edited

I've now merged latest apprtc and fixed some build steps and error handling.
Latest is pushed for testing here: https://callstats-dot-apprtc.appspot.com.

Any objections merging this? @juberti

@vr000m
vr000m commented Apr 18, 2016 edited

I presume that the callstats.min.js is cached/served by apprtc. Is it updated whenever the app is redeployed? (i.e., it pulls the latest copy from api.callstats.io)?

Further, would announcing updates on https://groups.google.com/forum/#!forum/callstats-dev be sufficient, to trigger an update on the cached JS? (@KaptenJansson, @juberti)

@KaptenJansson
Member

I presume that the callstats.min.js is cached/served by apprtc. Is it updated whenever the app is redeployed? (i.e., it pulls the latest copy from api.callstats.io)?

Yes that is correct, it's downloaded every time grunt build is executed (it's always executed prior deploying).

Further, would announcing updates on https://groups.google.com/forum/#!forum/callstats-dev be sufficient, to trigger an update on the cached JS? (@KaptenJansson, @juberti)

Yes that will do fine.

KaptenJansson added some commits May 18, 2016
@KaptenJansson KaptenJansson fix rejoin error and add githash to callstat configParams.
5450321
@KaptenJansson KaptenJansson Merge branch 'master' into callstats
81157d6
@KaptenJansson KaptenJansson Merge branch 'master' into callstats
2b4ef57
@KaptenJansson
Member

I'm going to merge this now and push this. Please file new issues if something is not working properly.

@KaptenJansson KaptenJansson merged commit f5aae01 into master Jul 6, 2016

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@KaptenJansson KaptenJansson deleted the callstats branch Jul 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment