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

alias_method vs prepend infinite looping #352

Closed
sribalakumar opened this issue Sep 1, 2020 · 3 comments
Closed

alias_method vs prepend infinite looping #352

sribalakumar opened this issue Sep 1, 2020 · 3 comments

Comments

@sribalakumar
Copy link
Contributor

When opentelemetry-ruby instrumentations is run in parallel with other Gems (like prometheus-exporter) which use a different patching technique and patch the same low level adapter method, we observe SystemStackError (stack level too deep) error at runtime.

More details related to SystemStackError originating because of patching conflicts between alias_method vs Module#prepend can be read from https://blog.newrelic.com/engineering/ruby-agent-module-prepend-alias-method-chains/

The patching problem between both techniques looks to a common issue among the APM providers community.
- elastic/apm-agent-ruby#379
- https://docs.newrelic.com/docs/agents/ruby-agent/troubleshooting/systemstackerror-stack-level-too-deep
- scoutapp/scout_apm_ruby#309 (data dog integration)
- DataDog/dd-trace-rb#862

May be we can provide a config toggle flag for the end user to choose which technique they need similar to New Relic implementation ?

New Relic config reference:
https://docs.newrelic.com/docs/agents/ruby-agent/configuration/ruby-agent-configuration

If prepend_active_record_instrumentation: true, uses Module.prepend rather than alias_method for ActiveRecord instrumentation.

@sribalakumar sribalakumar changed the title alias_method vs prepend looping alias_method vs prepend infinite looping Sep 1, 2020
@lloeki
Copy link

lloeki commented Nov 30, 2020

I encourage you to read this feedback I made to APM vendors: elastic/apm-agent-ruby#379 (comment)

tl;dr: just use prepend.

FYI list of instrumentors now using prepend:

  • scout_apm >= 2.5.2
  • ddtrace >= 0.27
  • skylight >= 5.0.0.beta
  • elastic-apm >= 4.0 (upcoming)
  • airbrake >= 11.0.2
  • newrelic_rpm >= 6.14 (partially: Rack's to_app is still alias method chain'ed, and ActiveRecord is behind prepend_active_record_instrumentation that as of today defaults to false)

At Sqreen we went the extra mile and detect those to fall back to alias method chain, or warn and bail out when there's a mix, but as of today just using prepend consistently is the solid choice.

@miquelbar
Copy link

I was being able to execute Opentelemetry with Prometheus exporter using the latest version of Prometheus Exporter. The latest version of Prometheus Exporter seems to work also with prepend: https://github.com/discourse/prometheus_exporter#choosing-the-style-of-method-patching

@ahayworth
Copy link
Contributor

We only have one instance of alias_method remaining in the codebase at this point. I think we're alright to close this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants