-
Notifications
You must be signed in to change notification settings - Fork 5
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
Refactor redis brain storage of incident data #14
Comments
This was referenced Jul 20, 2016
Closed
jm-welch
added a commit
to jm-welch/hubot-victorops
that referenced
this issue
Jul 20, 2016
fixes victorops#14, closes victorops#13 - Added new environment variable (HUBOT_VICTOROPS_KEEP) to control how long incidents are stored in the brain - Moved incident list to _private.VO_INCIDENTS, instead of as children of _private - Now using .getTime() to compare timestamps for incident expiry - Now using timestamps within the JSON data for each alert, rather than maintaining a separate list (VO_INCIDENT_KEYS) - Bot will log how many days incidents are to be kept on initialization
This was referenced Jul 20, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The current storage structure for incident data in the redis brain looks like this:
This seems redundant. Rather than writing one list that we then use to prune items from a separate dict, we should just maintain a single dict. The reason for the
VO_INCIDENT_KEYS
branch seems to be to allow us to timestamp items as we receive them, so that "old" items can be removed, but theTIMESTAMP
value within each incident's JSON data should serve the same purpose.I'd propose that this be restructured to look like this:
I added the CONTROL_CALL branch, as that's the next big thing I'm working on, and an example of data that would be useful to store, but more clumsy to access/iterate over if all the incidents are stored with it at that level in the data struct.
This will involve rewriting a couple of small pieces of the adapter, but should be doable. I'm already working on a rewrite, but won't have results until I finish my testing on #13.
Only major consideration at the moment is whether this is more computationally costly than the current method, as we'll be iterating over a dict of variable-size dicts, instead of a list of two-value dicts. I'm honestly not sure how to do performance benchmarking in coffeescript/nodejs, so once I have something working, I may need some help figuring out how to test that part.
The text was updated successfully, but these errors were encountered: