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

persisting measured jobs in redis #34

Closed
wants to merge 3 commits into from

Conversation

shivakumaarmgs
Copy link

@shivakumaarmgs shivakumaarmgs commented Dec 6, 2018

Linking #32

Reason: Without this change, only one or two job stats are shown on the page and they too are gone if we restart the workers. This is because the measured jobs class names are stored in an instance variable.

Made changes to store the measured jobs names in redis so that Job_Stats page shows the stats for all the Jobs ever recorded. This will fix #26.

Also fixing #21.
Need idea on solving the following scenarip:
A super-class extends just the JobStats::Duration module and if that super-class is inherited by a sub-class, that sub-class gets extended by JobStats module instead of JobStats::Duration module.

These changes are currently being used in our production without any issues.

@baarkerlounger
Copy link

@lukeasrodgers are you maintaining this repo? Did you ever get a chance to look at this?

@lukeasrodgers
Copy link
Collaborator

@baarkerlounger I am still maintaining it but (obviously) not super actively and haven't been able to put much time into reviewing this PR, was initially a bit leery about running the const_get on whatever is in redis, which is a possible security vulnerability, e.g. see https://blog.convisoappsec.com/en/exploiting-unsafe-reflection-in-rubyrails-applications/ and https://www.praetorian.com/blog/ruby-unsafe-reflection-vulnerabilities?edition=2019

I appreciate the problem this PR is solving but I think the security risks are not worth it here.

Thanks @shivakumaarmgs and very sorry for the delay here.

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.

Probably not threadsafe
3 participants