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

Refactor GuidGenerator to use SecureRandom (Fly backport) #2362

Open
fallwith opened this issue Dec 13, 2023 · 1 comment
Open

Refactor GuidGenerator to use SecureRandom (Fly backport) #2362

fallwith opened this issue Dec 13, 2023 · 1 comment
Labels
needs review version: 10.0.0 Next Major Release - Breaking Changes

Comments

@fallwith
Copy link
Contributor

The NewRelic::Agent::GuidGenerator module's generate_guid method uses a mixture of rand and rjust and has a source code comment that explains why SecureRandom is avoided.

With the Ruby versions supported by the current version of the agent, we can now consider the source code comment's cautions about running out of file descriptors to be outdated because of Bug #9569's resolution 7 years ago.

If we replace rand and rjust with SecureRandom.hex, we stand to gain a performance boost.

  module_function

- MAX_RAND_16 = 16**16
- MAX_RAND_32 = 16**32
- # This method intentionally does not use SecureRandom, because it relies
- # on urandom, which raises an exception in MRI when the interpreter runs
- # out of allocated file descriptors.
- # The guids generated by this method may not be _secure_, but they are
- # random enough for our purposes.
  def generate_guid(length = 16)
-   guid = case length
-          when 16
-            rand(MAX_RAND_16)
-          when 32
-            rand(MAX_RAND_32)
-          else
-            rand(16**length)
-   end.to_s(16)
-   guid.length < length ? guid.rjust(length, '0') : guid
+   SecureRandom.hex(length / 2)
  end

to test this change, here is a benchmark script:

require 'benchmark'
require 'securerandom'

class GuidGenerator
  MAX_RAND_16 = 16**16
  MAX_RAND_32 = 16**32

  def self.generate_with_rand(length = 16)
    guid = case length
           when 16
             rand(MAX_RAND_16)
           when 32
             rand(MAX_RAND_32)
           else
             rand(16**length)
    end.to_s(16)
    guid.length < length ? guid.rjust(length, '0') : guid
  end

  def self.generate_with_securerandom(length = 16)
    SecureRandom.hex(length / 2)
  end
end

TIMES = 1_000_000
LENGTHS = [16, 32, 24]

Benchmark.bm do |x|
  x.report 'with rand' do
    LENGTHS.each do |length|
      TIMES.times { GuidGenerator.generate_with_rand(length) }
    end
  end

  x.report 'with SecureRandom' do
    LENGTHS.each do |length|
      TIMES.times { GuidGenerator.generate_with_securerandom(length) }
    end
  end
end

which on my archaic machine produced the following numbers:

                   user     system      total        real
with rand          1.854185   0.007832   1.862017 (  1.864171)
with SecureRandom  1.282791   0.004168   1.286959 (  1.288676)
@workato-integration
Copy link

@kaylareopelle kaylareopelle added the version: 10.0.0 Next Major Release - Breaking Changes label Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review version: 10.0.0 Next Major Release - Breaking Changes
Projects
None yet
Development

No branches or pull requests

3 participants