Use #prepend over #alias_method for Thread plugin #1018

delner · 2020-12-04T19:34:05Z

Description of the change

The Thread plugin uses alias_method rewrites to patch Thread behavior. This is destructive and doesn't play well with other patches (particularly from other libraries.) Instead, use prepend which non-destructively patches.

While alias_method was previously necessary for Ruby < 2.0 support, with that version support being dropped, prepend should be preferred.

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

(None)

Checklists

Development

Lint rules pass locally
The code changed/added as part of this pull request has been covered with tests
All tests related to the changed code pass in development

Code review

This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
"Ready for review" label attached to the PR and reviewers mentioned in a comment
Changes have been reviewed by at least one other engineer
Issue from task tracker has a link to this pull request

delner

Just some notes for the code reviewer's consideration!

delner · 2020-12-04T19:35:14Z

lib/rollbar/plugins/thread.rb

-    Thread.class_eval do
-      def initialize_with_rollbar(*args, &block)
+  module Rollbar
+    module ThreadPlugin


I would have named this Thread, however, many other parts of the codebase use Thread instead of ::Thread, so this causes a naming conflict. Rather than update all those files, I opted to choose a different name.

delner · 2020-12-04T19:36:26Z

lib/rollbar/plugins/thread.rb

    end
  end
+
+  execute do
+    Thread.send(:prepend, Rollbar::ThreadPlugin) # rubocop:disable Lint/SendWithMixinArgument


Because Rollbar supports Ruby 2.0, you cannot do Thread.prepend, as the method is private in 2.0. Using send makes this compatible.

delner · 2020-12-04T19:37:02Z

spec/rollbar/plugins/thread_spec.rb

+
+Rollbar.plugins.load!
+
+describe Rollbar::ThreadPlugin do


There was no spec specifically for this plugin, so I thought I'd add this to improve coverage.

delner · 2020-12-09T18:46:44Z

Hey @waltjones! Would you be the right guy to ask for a code review? (Saw you had done the most recent release, so thought I'd ask.) I think this one should be ready for a review from your team 😄

waltjones · 2020-12-09T19:17:49Z

@delner Yes, happy to take a look at this.

waltjones

Looks good, and thank you for the helpful comments.

GaurabAryal · 2020-12-22T16:06:26Z

Hey @waltjones, I was wondering when the next build will be released with this fix included?

waltjones · 2020-12-22T16:17:11Z

It'll be in the next release. There isn't a date set for that, but most likely early January.

Rollbar gem versions <= 3.1.1 have an incompatibility with ddtrace's CPU profiling. This was ack'd and fixed upstream in <rollbar/rollbar-gem#1018>. This commit adds code to detect when customers are still using a legacy version of rollbar, printing a warning to ask them to upgrade. Note: Because the fixed version of rollbar has not yet been released, the tests that validate that the warning is not shown on unpatched versions are disabled and tagged with a FIXME. See the "FIXME NEW ROLLBAR NEEDED" note in the setup_spec.rb for more details on what's disabled and what steps we need to take once the new version gets released.

Rollbar gem versions <= 3.1.1 have an incompatibility with ddtrace's CPU profiling. This was ack'd and fixed upstream in <rollbar/rollbar-gem#1018>. This commit adds code to detect when customers are still using a legacy version of rollbar, printing a warning to ask them to upgrade. Note: Because the fixed version of rollbar has not yet been released, the tests that validate that the warning is not shown on unpatched versions are disabled and tagged with a FIXME. See the "FIXME NEW ROLLBAR NEEDED" note in the setup_spec.rb for more details on what's disabled and what steps we need to take once the new version gets released. First commit @ Datadog 🎉

mriddle · 2021-01-21T14:47:15Z

@waltjones any update on the release? 🙂

waltjones · 2021-01-21T15:14:06Z

@mriddle Not yet. Working to get a Rollbar.js release out any day now, and then will focus on rollbar-gem.

Rollbar gem versions <= 3.1.1 have an incompatibility with ddtrace's CPU profiling. This was ack'd and fixed upstream in <rollbar/rollbar-gem#1018>. This commit adds code to detect when customers are still using a legacy version of rollbar, printing a warning to ask them to upgrade. Note: Because the fixed version of rollbar has not yet been released, the tests that validate that the warning is not shown on unpatched versions are disabled and tagged with a FIXME. See the "FIXME NEW ROLLBAR NEEDED" note in the setup_spec.rb for more details on what's disabled and what steps we need to take once the new version gets released. First commit @ Datadog 🎉

waltjones · 2021-02-10T21:37:30Z

Released in v3.1.2. Apologies for the delay.

When I initially worked on #1300 ("Warn on incompatible rollbar version") my understanding of the issue on rollbar/rollbar-gem#1018 was incomplete and I did not realize that old rollbar versions, beyond breaking datadog profiler's CPU time instrumentation, could actually break the customer's application. One of our private beta customers mentioned he had seen both the warning but also had seen a stack trace: ``` [1] ! Unable to load application: SystemStackError: stack level too deep /usr/local/bundle/gems/rollbar-3.1.1/lib/rollbar/plugins/thread.rb:5:in `initialize_with_rollbar': stack level too deep (SystemStackError) from /usr/local/bundle/gems/ddtrace-0.45.0.feature.profiling.109457/lib/ddtrace/profiling/ext/cthread.rb:47:in `initialize' from /usr/local/bundle/gems/rollbar-3.1.1/lib/rollbar/plugins/thread.rb:6:in `initialize_with_rollbar' from /usr/local/bundle/gems/ddtrace-0.45.0.feature.profiling.109457/lib/ddtrace/profiling/ext/cthread.rb:47:in `initialize' ``` This led me to try to reproduce this issue. I discovered that with ```ruby ENV['DD_PROFILING_ENABLED'] = 'true' require 'ddtrace/profiling/preload' require 'rollbar' Rollbar.configure do |c| c.access_token = "nope" end Thread.new { }.join ``` I could trigger the issue, and that the current behavior was that the warning from #1300 would be printed, but then the application would proceed to fail. Having a bit more experience with the codebase now, I realize that the correct place to put this check is in the CPU extension #unsupported_reason method, which will ensure the extension is not loaded at all if an incompatible version of rollbar is around. The resulting behavior is that even with an old rollbar version, profiler will happily load and work; it will just omit the CPU time profiling, similarly to how it behaves on other platforms where for different reasons we don't support CPU time profiling.

ivoanjo · 2021-02-11T15:06:17Z

Thanks a lot for the effort @waltjones 👍 👍 👍