search

scoutapp / scout_apm_ruby

ScoutAPM Ruby Agent. Supports Rails, Sinatra, Grape, Rack, and many other frameworks
https://scoutapm.com
Other
198 stars 97 forks source link

Should not use alias_method #114

Closed tkawa closed 2 years ago

tkawa commented 7 years ago

It is risky if any other gems use prepend against the same method. https://github.com/scoutapp/scout_apm_ruby/blob/master/lib/scout_apm/tracer.rb#L63-L64

see also: https://speakerdeck.com/a_matsuda/ruby-2-in-ruby-on-rails (page 37-43)

It's dangerous to use both AMC and prepend against a single method The result is not only unpredictable Sometimes critical ☠️

eduardohertz commented 7 years ago

I've got a RuntimeError (can't modify frozen SystemStackError) error using scout_apm and jb gem.

jlsync commented 6 years ago

I've also got a problem with jb gem, some infinite recursion between scout and jb. Using prepend or perhaps just super might solve this problem.

cc: @itsderek23 

[7cb0f721-c3cc-4cf2-b186-7d11986b5f8e] jb (0.5.0) lib/jb/action_view_monkeys.rb:47:in `render_template'
[7cb0f721-c3cc-4cf2-b186-7d11986b5f8e] scout_apm (2.4.7) lib/scout_apm/tracer.rb:72:in `block in render_template_with_scout_instrument'
[7cb0f721-c3cc-4cf2-b186-7d11986b5f8e] scout_apm (2.4.7) lib/scout_apm/tracer.rb:35:in `instrument'
[7cb0f721-c3cc-4cf2-b186-7d11986b5f8e] scout_apm (2.4.7) lib/scout_apm/tracer.rb:68:in `render_template_with_scout_instrument'
[7cb0f721-c3cc-4cf2-b186-7d11986b5f8e] jb (0.5.0) lib/jb/action_view_monkeys.rb:47:in `render_template'
cschneid commented 6 years ago

This error is tricky.

The "correct" method of fixing this generally is to use Module#prepend, which is a fairly straightforward change to the instrumentation.

The issue is that we support rubies 1.8.7 and 1.9.3 still, and have customers actively using those versions. The older versions of ruby don't support Module#prepend, and so we've written all the instrumentation with the older alias_method approach. Either approach works great, but they don't interact well.

A temporary fix is to turn off loading the view instruments from Scout that are interfering with jb. This will cause View time to be reallocated to the wrapping controller time. Total time, database, and other tracing will be unaffected.

Add this to your config:

common: &defaults
  # ... 

  disabled_instruments:
    - ActionView

Longer term, we're looking at a way to have dual implementations of instruments, one using prepend, and one using alias. That would allow us to use the prepend version when the underlying ruby supports it.

jlsync commented 6 years ago

Yes, this is what I've done. Disabled ActionView instrumentation and then deployed jb (totally worth it)

replaced-jbuilder-with-jb
qinmingyuan commented 6 years ago

how about do not support ruby1.9.3 and 1.8 any more~

eduardohertz commented 6 years ago

@cschneid does PR #187 fix this problem?

cschneid commented 6 years ago

@eduardohertz not yet, but it's headed that way. We have a lot of instruments, and we're working on shifting them over.

jlsync commented 5 years ago

The latest scout_apm gem release ( with PR #255 included) is now working well for me with rails 5.2.x , ruby 2.5.x and jb gem.

cschneid commented 5 years ago

@jlsync excellent! I am not going to close this issue because several other instruments are still using alias_method