Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spine add stats and ES6 refactor #1014

Merged
merged 4 commits into from Aug 24, 2017
Merged

Conversation

lodoyun
Copy link
Contributor

@lodoyun lodoyun commented Aug 24, 2017

Description
This PR implements:

  • Spine is now updated to ES6.
  • Spine is now a different object that is used by runSpineClients.
  • Stats and Stream status can now be gathered when running a Spine Session.

You can specify an array of stat keys, a stat gathering interval and total number of intervals in spineClientsConfig.json The result will be written to a file that can be specified as a command line parameter for runSpineClients. The result will always be the output from the last stats gathering.

[] It needs and includes Unit Tests

Changes in Client or Server public APIs

[] It includes documentation for these changes in /doc.

Copy link
Contributor

@jcague jcague left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! just some minor comments

erizo_controller/erizoClient
spine/erizofc.js
erizo_controller/erizoClient/**/*.js
spine/**/*.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to remove spine from the folders that jshint inspect (in package.json).

In any case, we should run eslint for spine then. Otherwise, we will stop checking this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It helps when you have an editor configured with both eslint and jshint.
The idea is to create a gulp task or similar in spine to lint it, will do in a future PR.

@@ -0,0 +1 @@
./**/*.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above. Anyway, do we need it?

Erizo = require('./erizofc'),
NativeStream = require ('./NativeStream.js'),
NativeStack = require ('./NativeStack.js');
const XMLHttpRequest = require('xmlhttprequest').XMLHttpRequest; // eslint-disable-line import/no-extraneous-dependencies
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add a package.json in spine in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should, i'd do it when we create a package.json for each of the parts of licode.

publishConfig: streamConfig.publishConfig,
serverUrl: streamConfig.basicExampleUrl
};
streamConfig = require(`./${streamConfig}`); // eslint-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are other ways to read the config file better than require, we can fix it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

if (stream.pc && stream.pc.peerConnection && stream.pc.peerConnection.connected) {
promises.push(stream.pc.peerConnection.getStats());
} else {
log.info('No stream to ask for stats: ', streamId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log.warn?

that.pcConfig.iceServers = configuration.iceServers;
}

if (configuration.audio === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider configuration.audio = configuration.audio || false

configuration.audio = false;
}

if (configuration.video === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider configuration.video = configuration.video || false

@lodoyun lodoyun merged commit adcf2f5 into lynckia:master Aug 24, 2017
@lodoyun lodoyun deleted the add/SpineRefactor branch September 18, 2017 13:01
Arri98 pushed a commit to Arri98/licode that referenced this pull request Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants