igorkasyanchuk / rails_performance

Monitor performance of you Rails applications (self-hosted and free)
https://www.railsjazz.com/
MIT License
957 stars 54 forks source link

Crashes on Rails 5.0.x due to modulo of ActiveSupport::Duration returning integer #3

Closed synth closed 4 years ago

synth commented 4 years ago

In Rails 5.0.x (not sure about 5.1 or 5.2), when you modulo two ActiveSupport::Duration's, Rails returns an integer, not an ActiveSupport::Duration. This causes rails_performance to crash when accessed.

See here:

Screenshot 2020-05-26 20 06 16
igorkasyanchuk commented 4 years ago

@synth can you make a fix? it looks like a very simple fix, and it would be great if you can make it.

Sample: https://github.com/igorkasyanchuk/rails_db/blob/rails_4_x/lib/rails_db/table.rb#L65

synth commented 4 years ago

I spent an hour trying to do it, but I'm not too familiar with ActiveSupport::Duration's nor the internals of this gem. Can you provide some insight on what Utils.days is and how it's used?

I tried

      ActiveSupport::Duration.parse("PT#{RP.duration % 24.days}S").parts[:days] + 1

but there is no days part.

In Rails 5.0.x RP.duration % 24.days is 14400 in seconds, so I'm pretty confused on why this references the :days key in parts.

igorkasyanchuk commented 4 years ago

image maybe you can just use source code ? I mean just add this method dynamically to the ActiveSupport::Duration and patch it (backport)? if Rails version is 5.0

it basically counts number of the days added to the configuration

synth commented 4 years ago

what is the significance of 24.days? I'm confused on why you would mod 4.hours with 24.days...Should this be 24.hours instead?

And also why do you add +1?

It will also help me if you can clarify what the expected default return value is. Is it 14400 seconds (4 hours)?

igorkasyanchuk commented 4 years ago

If you have 4 hours it will return 0+1 If you have 32 hours it will be 1+1 I need to count number of days covered in range, and minimum is 1

synth commented 4 years ago

Right, so if you are counting days, it should be 24.hours, not 24.days. I'll try to put some time into this today and see what I can come up with.

synth commented 4 years ago

Ok, proposed fix in #4