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

Add loaded specs fallback #1510

Merged
merged 7 commits into from
May 20, 2021
Merged

Add loaded specs fallback #1510

merged 7 commits into from
May 20, 2021

Conversation

ericmustin
Copy link
Contributor

@ericmustin ericmustin commented May 19, 2021

this pr addresses #1505. Within the core tracer we should generally try to avoid Gem.loaded_specs if a known libraries version constant exists, as loaded_specs may not work depending on how the user is using Rubygems.

This work builds on top of #1506 and the next step here would be to add fallback code, where possible, for each contrib integration. Doing this repeatedly would be a chore so it's preferable to have a helper class for this that safely checks the version constant, maybe something like

module Datadog
  module Utils
    # Common gem-related utility functions.
    module Gem
      module_function

      # Safely falls back to checking if a gem has been loaded
      def loaded_specs_present?(gem_name = '', &block)
        begin
          Gem.loaded_specs[gem_name] || block.call

        # catch errors for an undefined or unsupported version constant
        rescue NameError, ArgumentError
        end
      end


      # Safely falls back to checking a gem's version constant if defined
      def loaded_specs_version(gem_name = '', &block)
        begin
          (Gem.loaded_specs[gem_name] && Gem.loaded_specs[gem_name].version) || Gem::Version.new(block.call)

        # catch errors for an undefined or unsupported version constant
        rescue NameError, ArgumentError
        end
      end
    end
  end
end
Datadog::Utils::Gem.loaded_specs_version('dogstatsd-ruby') { Datadog::Statsd::VERSION }

@ericmustin ericmustin requested a review from a team May 19, 2021 20:32
@codecov-commenter
Copy link

Codecov Report

Merging #1510 (7ab4f8d) into master (88d321b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1510      +/-   ##
==========================================
- Coverage   98.22%   98.22%   -0.01%     
==========================================
  Files         862      862              
  Lines       41686    41685       -1     
==========================================
- Hits        40945    40944       -1     
  Misses        741      741              
Impacted Files Coverage Δ
lib/ddtrace/metrics.rb 96.52% <100.00%> (ø)
spec/ddtrace/metrics_spec.rb 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88d321b...7ab4f8d. Read the comment docs.

@ivoanjo
Copy link
Member

ivoanjo commented May 20, 2021

I'm... still a bit unconvinced that this is correct:

~/datadog/dd-trace-rb$ bundle exec ruby -e "puts defined?(Datadog::Statsd::VERSION).inspect; puts Gem.loaded_specs['dogstatsd-ruby']"
nil
#<Bundler::StubSpecification name=dogstatsd-ruby version=5.0.1 platform=ruby>

But I admit some of the details of the inner workings of rubygems and Ruby are unknown to me. My understanding is that Gem.loaded_specs is available as long as Ruby is booted with rubygems enabled (which is the default); but the version constant only really shows up after the gem is required (or at least, after the file containing the version is required).

So I think this replacement is not a direct equivalent. This is why in #1506 I've tried to wire things up such as if the library appears to be loaded (and protobuf doesn't provide a version constant, so "appears to be loaded" was what I had to work with), then we avoid the use of Gem.loaded_specs, but there's a fallback to Gem.loaded_specs whenever the gem was not yet loaded.

@ericmustin
Copy link
Contributor Author

@ivoanjo yes i've been thinking about this incorrectly, will update issue + this pr

@ericmustin
Copy link
Contributor Author

@ivoanjo what do u think now, better?

lib/ddtrace/metrics.rb Outdated Show resolved Hide resolved
@ericmustin ericmustin added the dev/refactor Involves refactoring existing components label May 20, 2021
@ericmustin ericmustin added this to the 0.50.0 milestone May 20, 2021
@ericmustin ericmustin merged commit 01eea6a into master May 20, 2021
@ericmustin ericmustin deleted the add_loaded_specs_fallback branch May 20, 2021 22:07
@ivoanjo
Copy link
Member

ivoanjo commented May 21, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev/refactor Involves refactoring existing components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants