spies should use Module.prepend vs alias

[timogoebel]

Using Module.prepend is the default to override a method in Rails. Prior to that alias_method_chain.

We've been trying to use the APM with Foreman, which overrides net-http's request method as well as apm-agent-ruby. Foreman uses Module.prepend, this gem uses alias. And this leads to a SystemStackError (stack level too deep) exception.

As a workaround we currently disable the net-http spy in the config:

disabled_spies:
  - json
  - net_http

Can we switch to Module.prepend as a default or make this configurable?

[mikker]

Yes, we should definitely do this.

Thanks for bringing it up. I'll see to it when I find the time. PRs are of course welcome if you're impatient 😅

[mcfearsome]

We are also experiencing this, but with the elasticsearch spy

[mikker]

@mcfearsome Are you doing your own prepending? Or do you use a library that prepends?

[mikker]

I reverted this fix as it interfered with another library that didn't use Module.prepend.

I hope I'm wrong but I think we either choose to Module.prepend and then only work with libraries that use that too OR use alias and only work with other libraries that use that too.

[bencrouse]

Hi @mikker, we're prepending on our own to add a query-caching middleware in our app.

I thought I'd check to see what a similar gem like New Relic does, but it seems like they're also using alias: https://github.com/newrelic/rpm/blob/c9a467d22e6d4b893be33be330d258a125cf813a/lib/new_relic/agent/instrumentation/net.rb

I took a quick look, but couldn't figure out when the execute block runs there, maybe the defer/execute stuff was written to get around this?

[mikker]

I'm not familiar with their defer thing but I suspect it's their solution to something that we do as well: Modifying the constant if exists, otherwise add a hook to do it when required.

Anyway – you see our problem then. If somebody wanted to use both Elastic APM and NR, then Module#prepend would break that.

It seems though that they've written up a whole post on this and you might be able to solve it by loading things in the right order: https://blog.newrelic.com/engineering/ruby-agent-module-prepend-alias-method-chains/

Let me know how it works out if you decide to try.

[lloeki]

Feedback from the trenches:

I'm not familiar with their defer thing but I suspect it's their solution to something that we do as well: Modifying the constant if exists, otherwise add a hook to do it when required.

That's exactly it.

Anyway – you see our problem then. If somebody wanted to use both Elastic APM and NR, then Module#prepend would break that.

At Sqreen I've been in charge of moving our Ruby agent to Module#prepend earlier this year, because of conflicts with Scout and DataDog APMs (which moved to prepend too). Skylight has moved to prepend as well on their master, released as beta. NewRelic is using prepend, except in one case.

It seems though that they've written up a whole post on this and you might be able to solve it by loading things in the right order: https://blog.newrelic.com/engineering/ruby-agent-module-prepend-alias-method-chains/

Let me know how it works out if you decide to try.

I can tell you that. The following workaround may work, but it really depends on -when- the alias method chain stuff is called. Hopefully it's called early enough because the consts are defined by the time the app initializers are reached. -Hopefully-.

# Gemfile
gem 'sqreen', require: false  # or the gem using prepend

# Rails config/application.rb
module DemoApp
  class Application < Rails::Application
    # other settings redacted for brevity...
    initializer :sqreen do |app|
      require 'sqreen'
    end    
  end
end

The thing is, alias method chaining has too much issues and doesn't always work even with other alias method chain hacks since it just can't know where it is in the fake inheritance chain, which can produce many results from infinite call loops to simply one obliterating the other in perfect silence. I've fought those battles first-hand, they're not worth fighting. Module#prepend, in contrast, always works (except, of course with alias method chaining, but that's a core defect of the latter, not the former). After all, it is its very raison d'etre.

The way forward is Module#prepend. I can only encourage you to consider moving to it.

[mikker]

@lloeki Thank you for that thorough update. I completely agree and it's nice to see most gems are moving in that direction. I think we should follow suit, perhaps in the next major bump (like Skylight) because it still is a kind of breaking change, and in that way we won't have to end up having to support both approaches simultaneously.

Hopefully not breaking many setups, but the major bump will hopefully make people notice and take precautions if they mix the different gems.

[estolfo]

Spies are using Module.prepend as of this commit in the next branch. The change will be in the next major, version 4.0.

[lloeki]

Thanks for the update including the version to match, I'll update our dependency detector with the compatibility information so that the correct instrumentation method gets picked up.

[estolfo]

@lloeki thanks for your help!