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

Add ROV - The licode tool for inspection and app-level stats #1362

Merged
merged 11 commits into from
Feb 21, 2019

Conversation

lodoyun
Copy link
Contributor

@lodoyun lodoyun commented Feb 19, 2019

Description

This PR implements ROV - a way to inspect licode components and gathering app-level stats for prometheus.
This works by allowing running remote REPL sessions in all erizo server-side components (erizoController, erizoAgent and erizoJS). The main components are:

  • RovReplManager: It runs on every erizo component and allows remote REPL sessions to be run via rabbitmq.
  • RovClient: The client for RovReplManager. It can start remote repl sessions and run commands in them.
  • Inspector: It's a local Repl server that allows using rovClient directly.
  • Metrics:
    • RovMetricsGatherer: Gathers all metrics by issuing Rov Commands and processing them
    • RovMetricsServer: Sets up prometheus-client that can be used by a prometheus server to get the metrics from RovMetricsGatherer.

[] 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.

I really like this new component!

I left some comments, mostly related to an alternative use of Promises, but feel free to ignore them if they don't make sense.

Also, I'm not sure if using ROV instead of rov for the folder name follows our current naming convention for folder and so on, since we have lots of them in small case. But it's up to you.

@@ -24,6 +24,7 @@ describe('Erizo Agent', () => {
};

beforeEach(() => {
global.config = { logger: { configFile: true } };
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to enable logs for UTs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just needed global.config to be present correctly for the ROV componets, otherwise tests will fail. I noticed that's what we did for erizoController and ErizoJS so I followed that pattern.

this.components.erizoAgents.forEach((agent) => {
erizoListPromises.push(agent.runAndGetPromise('console.log(context.toJSON())'));
});
Promise.all(erizoListPromises).then((erizoResults) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider something like:

this._establishComponentConnections(this.components.erizoAgents).then(() => {
  ...
  return Promise.all(erizoListPromises);
}).then((erizoResults) => {
  ...
  resolve();
}).catch(() => {
  ...
});


runInComponentList(command, componentList) {
return new Promise((resolve, reject) => {
this.connectToComponentList(componentList).then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider something like:

runInComponentList(command, componentList) {
  return this.connectToComponentList(componentList).then(() => {
    ...
    return Promise.all(runPromises);
  });
}

});
}

connectToComponentList(componentList) {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider:

connectToComponentList(componentList) {
  ...
  return Promise.all(connectPromises);
}

return this.sessions;
}

connectToSession(componentId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider:

connectToSession(componentId) {
  ...
  if (this.sessions.has(componentId)) {
    return Promise.resolve();
  }
  ...
  return rovConnection.connect().then(() => {
    ...
    return Promise.resolve();
  });
}

});
}

closeAllSessions() {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider:

closeAllSessions() {
  ...
  return Promise.all(closePromises).then() => {
    ...
    return Promise.resolve();
  });
}

this.log = logger;
}

getTotalRooms() {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider:

getTotalRooms() {
  ...
  return this.rovClient.runInComponentList(...).then((result) => {
    ...
    return Promise.resolve();
  });
}

});
}

getTotalClients() {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider something similar to what I propose for getTotalRooms()

});
}

getActiveProcesses() {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just return Promise.resolve() here if you want to give it a Promise-like interface.

});
}

getTotalPublishersAndSubscribers() {
Copy link
Contributor

Choose a reason for hiding this comment

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

there is an alternative way, similar to what I proposed before.

@lodoyun lodoyun merged commit e8e5892 into lynckia:master Feb 21, 2019
@lodoyun lodoyun deleted the add/Rov branch February 21, 2019 15:53
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.

2 participants