-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: master
Are you sure you want to change the base?
Conversation
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 |
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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:
- 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. - 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.
There was a problem hiding this comment.
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.
Subscribe to global event rather that devDealer. Fix eslisnt errors.
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 fromuuid.v4
) and may overlap after some time.