-
Notifications
You must be signed in to change notification settings - Fork 0
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
RamSize plugin #98
RamSize plugin #98
Conversation
Looks pretty good. +1 |
@@ -49,7 +49,7 @@ The Cache | |||
~~~~~~~~~ | |||
|
|||
Our temporary cache is stored in a ``Cache`` object. The source code for this | |||
can be found in ``lib/cache.rb``. Basically all it does is: | |||
can be found in ``lib/util.rb``. Basically all it does is: |
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.
looks like this doesn't belong here
aside from the cache -> util changes that don't look like they belong in this PR |
I'm testing this right now and it seems like the ram_size plugin doens't store to the DB.
I'm investigating it now |
found it,
|
TL,DR: we need to change So it appears that you (and I, too) got confused from collectors.rb where
so to fix this bug, we need to change |
@alxngyn Good catch, I didn't think to look for that. I'll also add a test for that somewhere... |
@alxngyn I'm keeping the cache.rb -> util.rb in this PR. I ended up adding some util functionality that actually gets used in a test. I could add a separate util.rb from the cache.rb but I think it's best to keep them in the same file. |
@ElijahCaine, okay as long as the change (cache.rb -> utils.rb) is consistent throughout the app |
@alxngyn I checked to make sure it was consistent. |
Won't run because collectors.rb still has the cache reference:
|
Includes transition from lib/cache.rb to lib/util.rb
@alxngyn Made that change. I swear I thought I had completely converted to the new |
I just tested it. It runs but now the DB isn't being written to again
|
@alxngyn Weeeeird. I'll take another look at it. Thanks for the heads up. |
@alxngyn I pushed a fix. Turns out our collector was not parsing the JSON it was collecting. Honestly I don't know how anything was working. Should be good now though. |
Good fix! Expected output is good,
|
+1 |
+1 for me too |
fixes issue #75
Changes in this PR.
Testing this PR.
rake spec
Expected Output.
@osuosl/devs @Kennric