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

Device stats plugin feature #442

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

neofreko
Copy link

@neofreko neofreko commented Oct 7, 2016

This plugin store device metadata along startsFrom and EndsAt datetime for device usage. EndsAt datetime is updated every 5 minutes via setTimeout.

By using the stats data, we can derive chart of popular devices (model, OS, etc), how many minutes spend, and also most active user.

This kind of data will be useful to analyze the value of our openSTF installation.

Potential issue:
leaseId is not unique enough (eventhough generated from uuid.v4 ) and may overlap after some time.

This plugin store device metadata along startsFrom and EndsAt datetime for device usage. EndsAt datetime is updated every 5 minutes via setTimeout.
lib/db/api.js Outdated
return db.run(r.table('users').get(group.email))
.then(function(user) {
return db.run(r.table('deviceUsageStats').insert({
leaseId
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately our codebase hasn't been set up for ES6 features yet and Travis is complaining about it. Would you mind changing all of these to the old-style leaseId: leaseId for now?

var syrup = require('stf-syrup')

var logger = require('../../../../util/logger')
var dbapi = require('../../../../db/api')
Copy link
Member

Choose a reason for hiding this comment

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

So this actually goes against the current philosophy we have. But you couldn't have known that so I'll explain it to you. The idea is that the provider/device units do NOT ever connect to the database directly. The reasons are:

  1. The provider and device units were designed so that they could be run on, say, individual developer laptops. So, the developer could run stf provider --connect-sub ... to provide whatever local devices he/she has to the rest of the company. While STF is by no means a secure system, having direct database access from anyone's machine is a bit too much even for us. It's also a pain from a maintenance point of view, since people could be running different versions of the database driver.
  2. It makes it difficult for the rest of the system to hook into the events generated by your plugin, since it's modifying the database directly. For example, perhaps someone would want to capture and output log messages whenever you update the stats.

As you can see, all other device plugins push out messages to the processor via ZeroMQ. This means that only the app units, that can run in the cloud or on physical servers, have direct access to the database. Also, it makes it possible for other units to receive these messages if they wish. If you look at the reaper unit, it simply plugs into heartbeat events and requires absolutely no changes to the rest of the system. It's possible you may have to add the current owner to the heartbeat message, but other than that, I don't see a reason why you couldn't implement this device plugin as a complete unit, just like reaper.

Creating a separate unit would also have the benefit of running the new unit as a separate process, meaning that even before we merge the feature, you wouldn't have to run a fork of STF. You could simply run the public version and maintain a separate repository for your custom unit, which you would launch separately.

Other than that this is a very interesting feature and I hope you can refactor your PR to match the requirements above.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the kind elaboration.
I'll look into reaper unit to learn further.

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