AWS patcher loads wrong Client constant

[letiesperon]

So, I had a class called Client in my app.
Now I added the gem gem 'ddtrace', '~> 0.28.0' and I get this error:

Unable to apply AWS integration: undefined method add_plugin for Client (call 'Client.connection' to establish a connection):Class

Because it seems that this adds its own class called Client right? Which is overriden by my own Client class.

Why didn't you namespace it to be sth more specific like Datadog::Client ? I mean, Client is a pretty common name.

[delner]

Could you be more specific about how this error was produced? Do you have a snippet of code that will reproduce it?

Currently on master, we have two classes named Client, and both are namespaced as Datadog::Transport::HTTP::Client and Datadog::Transport::IO::Client. There are 3 modules as well, but those are namespaced and shouldn't create conflicts either.

[delner]

It's possible the error may stem from the AWS patcher code, which attempts to load Client constants from the AWS gem itself:

def patch
  require 'ddtrace/contrib/aws/parsed_context'
  require 'ddtrace/contrib/aws/instrumentation'
  require 'ddtrace/contrib/aws/services'

  add_plugin(Seahorse::Client::Base, *loaded_constants)
end

def add_plugin(*targets)
  targets.each { |klass| klass.add_plugin(Instrumentation) }
end

def loaded_constants
  SERVICES.each_with_object([]) do |service, constants|
    next if ::Aws.autoload?(service)
    constants << ::Aws.const_get(service).const_get(:Client) rescue next
  end
end

If I'm reading this error correctly, it sounds like one of the Client constants in the AWS gem does not implement add_plugin as expected.

Can you share:

• The gem version of aws-sdk and its sub-gems
• Your configuration file for ddtrace

Maybe with that we can try to replicate this; might require a bugfix to our patcher if so.

[illdelph]

Hi @delner

From our Gemfile.lock:
aws-eventstream (1.0.3) aws-partitions (1.252.0) aws-sdk-core (3.85.0) aws-eventstream (~> 1.0, >= 1.0.2) aws-partitions (~> 1, >= 1.239.0) aws-sigv4 (~> 1.1) jmespath (~> 1.0) aws-sdk-kms (1.27.0) aws-sdk-core (~> 3, >= 3.71.0) aws-sigv4 (~> 1.1) aws-sdk-s3 (1.59.0) aws-sdk-core (~> 3, >= 3.83.0) aws-sdk-kms (~> 1) aws-sigv4 (~> 1.1) aws-sigv4 (1.1.0) aws-eventstream (~> 1.0, >= 1.0.2)

ddtrace:
Datadog.configure do |c| c.use :rails c.use :aws c.use :redis c.use :sidekiq c.tracer enabled: !%w[development test].include?(Rails.env), hostname: '172.17.0.1', port: '8126' c.analytics_enabled = true end

Also when we renamed our Client class to something else temporarily, the error did not appear.

[letiesperon]

@delner any updates on this? Thank you very much

[delner]

Currently trying to reproduce this, will update afterwards. If you have sample code that reproduces the issue that'd be most welcome.

[illdelph]

Hi @delner

After doing some digging Client is being added to the list of loaded_constants because of the "AWS Support Service" (https://github.com/aws/aws-sdk-ruby/blob/master/gems/aws-sdk-support/lib/aws-sdk-support.rb).

::Aws.const_get("Support").const_get(:Client)
 => Client (call 'Client.connection' to establish a connection) 

vs S3

 ::Aws.const_get("S3").const_get(:Client)
 => Aws::S3::Client

All the other targets are properly nested except Support Client.

Seahorse::Client::Base
Aws::S3::Client
Client

[delner]

From what I can tell with the latest ddtrace and the latest AWS SDK (where aws-support-sdk is 1.17.0):

irb(main):004:0> ::Aws.const_get("Support").const_get(:Client)
=> Aws::Support::Client

Looks like everything is loading properly to me, no errors. What version of aws-support-sdk are you using? You might want to update your gems.

[illdelph]

Hi @delner

The problem was not with the AWS SDK but rather the way ddtrace adds AWS Services. By iterating through a list of services and running constants << ::Aws.const_get(service).const_get(:Client) rescue next, Support was erroneously recorded as a "loaded constant".

My app has a class called Client as well as a folder in /lib called /support. Rails autoloads /support as a module called Support. When ::Aws.const_get(:Support) is called in loaded_constants it succeeds but not with ::AWS::Support but rather Support from /support and when it calls .const_get(:Client) it doesn't trigger an exception because of our Client class.

Might it be better to get the intersection of the ddtrace SERVICES list and ::Aws.constants?

[delner]

@illdelph

When ::Aws.const_get(:Support) is called in loaded_constants it succeeds but not with ::AWS::Support but rather Support from /support

I'm not sure how this is happening. Module#const_get is only supposed to get constants from the namespace its called against. e.g. via Ruby 2.5.1 irb:

module Aws
  module Support
    class Client
    end
  end
end

module Support
  class Client
  end
end

[].tap do |loaded_constants|
  loaded_constants << ::Aws.const_get(:Support).const_get(:Client)
end
# => [Aws::Support::Client]

Maybe Rails auto-loading is involved, but I also ran this in a Rails 5.2.2 application after adding Support::Client to lib/support/client.rb and got the same result.

[illdelph]

@delner

Just to be clear, client.rb is a model (app/models/client.rb) and has nothing to do with the support folder.

Also if you have the aws-support-sdk gem, the correct module is used but my project does not use nor has the aws-support-sdk.

2.6.5 :001 > ::Aws.const_get(:Support)
 => Support 
2.6.5 :002 > ::Aws.const_get(:Support).const_get(:Client)
 => Client 

Here is the code I used to generate the above: https://github.com/illdelph/dd-test

[delner]

@illdelph Okay, I loaded up that repo and I can see the error now. I'm going to crack open the patcher with this example and see what I can make of this and how to fix it. Will get back to you; thanks!

[delner]

It looks like Module#const_get actually searches ancestors by default hence when it misses, it looks up a level and returns Support and your Client instead.

This is an unusual circumstance because you did not load the full aws-sdk gem and you happened to define a module that matched its own namespacing minus Aws, which explains why this was not seen earlier. If you had namespaced Client, I don't think this would have happened.

I think this should be relatively easy to fix; I'll make the constant resolution a little more restrictive so it doesn't search ancestors.

EDIT: It's even more unexpected than originally thought because calling const_get twice for the same thing actually returns different results:

[1] pry(Datadog::Contrib::Aws::Patcher)> ::Aws.const_get('Support', false)
=> Support
[2] pry(Datadog::Contrib::Aws::Patcher)> ::Aws.const_get('Support', false)
NameError: uninitialized constant Aws::Support

Meaning when we disable the ancestor search, it still doesn't work as expected. When I try to replicate this in irb (without Rails or AWS`) the behavior is consistent and expected:

class Bar
end

module Foo
  # class Bar
  # end
end

::Foo.const_get('Bar', false)
# => NameError (uninitialized constant Foo::Bar)
::Foo.const_get('Bar', false)
# => NameError (uninitialized constant Foo::Bar)

So there's something weird going on with Rails autoloading I'd suspect. Either way, we should be able to work around this.

[frsantos]

In new Rails 6 loader "zeitwerk", it says (https://github.com/fxn/zeitwerk#motivation):

On the other hand, autoloading in Rails is based on const_missing, which lacks fundamental information like the nesting and the resolution algorithm that was being used. Because of that, Rails autoloading is not able to match Ruby's semantics and that introduces a series of gotchas. The original goal of this project was to bring a better autoloading mechanism for Rails 6.

[delner]

@frsantos Good to know; thanks! Given this is a Rails 5 app, I'm not sure if zeitwerk is the problem, or if in fact is meant to resolve this problem. Useful context to have nonetheless.

[delner]

@illdelph and @letiesperon: This should be addressed by #945, please feel free to give it a try and let me know if it works for you.

[illdelph]

@delner I tried out the PR and it looks like the error is gone 👍

[delner]

Should be addressed when the next release rolls; thank you for the report and sample code to reproduce!