Fix issue: Wrong duration for DB transaction event on ROR 7.1 #92

[ibnjunaid]

Issue #, if available:

Description of changes:

X-Ray SDK expects time in seconds with up to nanosecond precision.
This change make sure that the time is passed in seconds to the X-Ray for DB transactions.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[wangzlei]

LGTM

[wangzlei]

The PR is to address DB subsegment timestamp issue in rails version larger than 7.
#92

[jj22ee]

Please take a look at my comments.

I believe you can check for rails version like:

if Rails::VERSION::MAJOR >= 7
  # Do the new thing
else
  # Do it the old way
end

[jj22ee]

TY, can you update convert_time_in_seconds to be specifically for Rails 7.1, and update unit tests appropriately? Seems we only want to divide by 1000 only if Rails version is 7.1.

[ibnjunaid]

convert_time_in_seconds

@jj22ee How do you suggest.

should we do something like this

          subsegment.start_time = (::Rails::VERSION::MAJOR == 7 and ::Rails::VERSION::MINOR == 1) ? convert_time_in_seconds(transaction.time.to_f) : transaction.time.to_f

or

   subsegment.start_time = (::Rails::VERSION::MAJOR == 7 and ::Rails::VERSION::MINOR == 1) ? transaction.time.to_f / 1000 : transaction.time.to_f

[ibnjunaid]

@jj22ee

I am in favour of the first one because the changes are being back ported to some rails 7.1 version so I think its not a good idea to just divide by 1000 without checking.

Like has been back ported to 7-1-stable rails/rails#50493 (comment)

https://api.rubyonrails.org/v5.1/classes/Rails/VERSION.html

subsegment.start_time = (::Rails::VERSION::MAJOR == 7 and ::Rails::VERSION::MINOR == 1) ? convert_time_in_seconds(transaction.time.to_f) : transaction.time.to_f

[jj22ee]

Like has been back ported to 7-1-stable rails/rails#50493 (comment)

1. This is saying the bug is back ported to 7.1, the fix isn't backported from looking at this issue
2. The function you are proposing will still have the code smell I've commented here

Can you provide some test results for Rails v7, v7.1, v7.2 for your change?

[jj22ee]

We can see here that the fix is applied in 7.1-stable branch here

[ibnjunaid]

@jj22ee . The issue is still there for the available versions of 7.1.x.y of rails .
Theres not any rails version with stable release it is just a branch in the repo as of now.
https://rubygems.org/gems/rails/versions