search

getsentry / sentry-ruby

Sentry SDK for Ruby
https://sentry.io/for/ruby
MIT License
933 stars 494 forks source link

Play nice with sorbet and tapioca #2189

Open Velrok opened 11 months ago

Velrok commented 11 months ago

Describe the idea

It would be nice if the monitor_check_ins.rb wouldn't use preprend, because that means we can't adopt it alongside sorbet atm.

Why do you think it's beneficial to most of the users

Sorbet is a mature type checker for ruby, which is great to have in large code bases. They decided not to support prepend, which seams reasonable to me.

Possible implementation

Sorry I don't understand prepend well enough to really comment. Couldn't it be wrapped in some sort of block / yield alternative?

sl0thentr0py commented 11 months ago

@Velrok some of our other patches (redis/net-http) also use prepend, how does that work then?

Velrok commented 11 months ago

@Velrok some of our other patches (redis/net-http) also use prepend, how does that work then?

Hmm not sure. This blows up when we try to run

bin/tapioca dsl

Which will start the rails app and inspect the classes at runtime.

Maybe the other patches are not loaded?

Is sorbet compatible something you are interested in supporting? If so I could probably negotiate some time to produce a minimal failing example.

sl0thentr0py commented 11 months ago

@Velrok I haven't really thought about it since you're the first person who asked. I can give it some thought if you give me a simple repro, yes. In general, we at sentry like to be as close to 'zero config' as possible so we do have to do some monkeypatching for that but if there's an easy way around I can see.

jgrau commented 9 months ago

This is not tested but I found myself in the same situation and thought about adding something like this to ApplicationJob: 

  sig { params(slug: String, monitor_config: Sentry::Cron::MonitorConfig).void }
  def self.sentry_monitor_check_ins(slug:, monitor_config:)
    around_perform do |_job, block|
      check_in_id = Sentry.capture_check_in(slug, :in_progress, monitor_config:)

      start = Sentry.utc_now.to_i

      begin
        block.call
        duration = Sentry.utc_now.to_i - start

        Sentry.capture_check_in(
          slug,
          :ok,
          check_in_id:,
          duration:,
          monitor_config:,
        )
      rescue StandardError
        duration = Sentry.utc_now.to_i - start

        Sentry.capture_check_in(
          slug,
          :error,
          check_in_id:,
          duration:,
          monitor_config:,
        )

        raise
      end
    end
  end

It

By having the same interface you should be able to replace that method with the sentry mixin if/when sentry starts playing nice with Sorbet.

In the end I made another solution so I never tested this in production. I did run some tests in development and that seemed to work just fine.

st0012 commented 8 months ago

I want to first clarify that while Sorbet doesn't support prepend, it simply means it can't properly infer the method when it's defined through prepend. It does NOT mean projects can't have any prepend if they use Sorbet. Even Tapioca itself uses prepend in a few places (example).

Secondly, if something crashes when running tapioca dsl, it has nothing to do with Sorbet. The command, as you said, loads the application and uses the runtime info to generate rbi files. If things go wrong here, it's purely a runtime issue, not related to Sorbet.

And since it's a runtime issue, it's likely caused by things not being loaded properly. The solution could be:

However, without further information we can't properly suggest a better solution here. Can you please add more information, like:

jgrau commented 8 months ago

@st0012 thanks for the feedback. I do believe the crash Sorbet+Tapioca users experience here has to do with prepend and is not a loading issue. Here's the information you requested:

Versions:

    sorbet (0.5.11287)
      sorbet-static (= 0.5.11287)
    sorbet-runtime (0.5.11287)
    sorbet-static (0.5.11287-universal-darwin)
    sorbet-static (0.5.11287-x86_64-linux)
    sorbet-static-and-runtime (0.5.11287)
      sorbet (= 0.5.11287)
      sorbet-runtime (= 0.5.11287)
    sorbet-struct-comparable (1.3.0)
      sorbet-runtime (>= 0.5)
    tapioca (0.12.0)

Error backtrace:

❯ task tapioca:dsl
task: [tapioca:dsl] bin/tapioca dsl
Loading DSL extension classes... Done
Loading Rails application... 
Tapioca attempted to load the Rails application after encountering a `config/application.rb` file, but it failed. If your application uses Rails please ensure it can be loaded correctly before generating RBIs. If your application does not use Rails and you wish to continue RBI generation please pass `--no-halt-upon-load-error` to the tapioca command in sorbet/tapioca/config.yml or in CLI.
You're trying to replace `perform` on `CacheListingMarketCommand::CronJob`, but that method exists in a prepended module (Sentry::Cron::MonitorCheckIns::Patch), which we don't currently support.
/Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/class_utils.rb:115:in `block in replace_method': You're trying to replace `perform` on `CacheListingMarketCommand::CronJob`, but that method exists in a prepended module (Sentry::Cron::MonitorCheckIns::Patch), which we don't currently support. (RuntimeError)
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/class_utils.rb:105:in `each'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/class_utils.rb:105:in `replace_method'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:257:in `_on_method_added'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:549:in `method_added'
        from /Users/jgrau/src/landfolk/apps/api/app/commands/cache_listing_market_command.rb:75:in `<class:CronJob>'
        from /Users/jgrau/src/landfolk/apps/api/app/commands/cache_listing_market_command.rb:66:in `<class:CacheListingMarketCommand>'
        from /Users/jgrau/src/landfolk/apps/api/app/commands/cache_listing_market_command.rb:7:in `<main>'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/3.3.0/bundled_gems.rb:74:in `require'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/bootsnap-1.18.3/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/kernel.rb:26:in `require'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/helpers.rb:135:in `const_get'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/helpers.rb:135:in `cget'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/eager_load.rb:175:in `block in actual_eager_load_dir'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/helpers.rb:40:in `block in ls'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/helpers.rb:25:in `each'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/helpers.rb:25:in `ls'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/eager_load.rb:170:in `actual_eager_load_dir'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/eager_load.rb:17:in `block (2 levels) in eager_load'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/eager_load.rb:16:in `each'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/eager_load.rb:16:in `block in eager_load'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/eager_load.rb:10:in `synchronize'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader/eager_load.rb:10:in `eager_load'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader.rb:379:in `block in eager_load_all'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader.rb:377:in `each'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/zeitwerk-2.6.13/lib/zeitwerk/loader.rb:377:in `eager_load_all'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca/loaders/loader.rb:206:in `eager_load_rails_app'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `bind_call'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `block in _on_method_added'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca/loaders/loader.rb:60:in `load_rails_application'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `bind_call'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `block in _on_method_added'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca/loaders/dsl.rb:84:in `load_application'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `bind_call'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `block in _on_method_added'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca/loaders/dsl.rb:29:in `load'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `bind_call'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `block in _on_method_added'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca/loaders/dsl.rb:22:in `load_application'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `bind_call'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `block in _on_method_added'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca/commands/abstract_dsl.rb:108:in `load_application'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `bind_call'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `block in _on_method_added'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca/commands/dsl_generate.rb:11:in `execute'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `bind_call'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `block in _on_method_added'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca/commands/command.rb:27:in `block in run'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca.rb:23:in `block in silence_warnings'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/3.3.0/rubygems/user_interaction.rb:46:in `use_ui'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca.rb:22:in `silence_warnings'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `bind_call'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `block in _on_method_added'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca/commands/command.rb:26:in `run'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `bind_call'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/sorbet-runtime-0.5.11287/lib/types/private/methods/_methods.rb:279:in `block in _on_method_added'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/lib/tapioca/cli.rb:168:in `dsl'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/thor-1.3.1/lib/thor/command.rb:28:in `run'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/thor-1.3.1/lib/thor/invocation.rb:127:in `invoke_command'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/thor-1.3.1/lib/thor.rb:527:in `dispatch'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/thor-1.3.1/lib/thor/base.rb:584:in `start'
        from /Users/jgrau/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/tapioca-0.12.0/exe/tapioca:25:in `<top (required)>'
        from /Users/jgrau/src/landfolk/apps/api/bin/tapioca:27:in `load'
        from /Users/jgrau/src/landfolk/apps/api/bin/tapioca:27:in `<main>'
task: Failed to run task "tapioca:dsl": exit status 1

Tapioca config:

gem:
  # Add your `gem` command parameters here:
  #
  exclude:
    []
    # - red-arrow
    # - red-parquet
  # doc: true
  # workers: 5
dsl:
  # Add your `dsl` command parameters here:
  #
  # exclude:
  # - SomeGeneratorName
  # workers: 5

The sorbet-runtime method that raises the error looks like this:

  # Replaces a method, either by overwriting it (if it is defined directly on `mod`) or by
  # overriding it (if it is defined by one of mod's ancestors).  If `original_only` is
  # false, returns a ReplacedMethod instance on which you can call `bind(...).call(...)`
  # to call the original method, or `restore` to restore the original method (by
  # overwriting or removing the override).
  #
  # If `original_only` is true, return the `UnboundMethod` representing the original method.
  def self.replace_method(mod, name, original_only=false, &blk)
    original_method = mod.instance_method(name)
    original_visibility = visibility_method_name(mod, name)
    original_owner = original_method.owner

    mod.ancestors.each do |ancestor|
      break if ancestor == mod
      if ancestor == original_owner
        # If we get here, that means the method we're trying to replace exists on a *prepended*
        # mixin, which means in order to supersede it, we'd need to create a method on a new
        # module that we'd prepend before `ancestor`. The problem with that approach is there'd
        # be no way to remove that new module after prepending it, so we'd be left with these
        # empty anonymous modules in the ancestor chain after calling `restore`.
        #
        # That's not necessarily a deal breaker, but for now, we're keeping it as unsupported.
        raise "You're trying to replace `#{name}` on `#{mod}`, but that method exists in a " \
              "prepended module (#{ancestor}), which we don't currently support."
      end
    end

    overwritten = original_owner == mod
    T::Configuration.without_ruby_warnings do
      T::Private::DeclState.current.without_on_method_added do
        def_with_visibility(mod, name, original_visibility, &blk)
      end
    end

    if original_only
      original_method
    else
      new_method = mod.instance_method(name)
      ReplacedMethod.new(mod, original_method, new_method, overwritten, original_visibility)
    end
  end

which leads me to think that the error is intentional and not a bug or a loading issue.

st0012 commented 7 months ago

Thank you for the information, I now understand the problem (also described in great detail in this issue). Unfortunately, I don't think not using prepend is possible because the alternative would be something like alias_method_chain, which when other gems use prepend, would cause infinite loop instead (which is also why it's a deprecated practice).

I think for now the only way to avoid this is to opt-out the patch that triggers the prepend. Given the class's name has CronJob, I assume you probably use sidekiq-cron? If you do, can you try adding this to your Sentry config file:

Sentry.init { ... } 
Sentry.registered_patches.delete(:sidekiq_cron)
sethherr commented 7 months ago

A related/separate disadvantage of prepend is that you can't inherit sentry_monitor_check_ins from parent classes.

For example:

class BaseScheduledJob
  sentry_monitor_check_ins

  def perform
    # this calls Sentry::Cron::MonitorCheckIns::Patch#perform
  end
end

class SomeSpecificScheduledJob < BaseScheduledJob
  def perform
    # this doesn't call Sentry::Cron::MonitorCheckIns::Patch#perform
  end
end

This is subtle and took a while for me to figure out (I learned some new things!). It makes me feel like prepend might not be the right solution here.

ghiculescu commented 4 days ago

Ended up using something based on @jgrau's snippet:

  # app/jobs/application_job.rb

  # Alternative to the same method provided in +Sentry::Cron::MonitorCheckIns+,
  # which doesn't work with Sorbet, see https://github.com/getsentry/sentry-ruby/issues/2189
  sig { params(monitor_config: Sentry::Cron::MonitorConfig, slug: T.nilable(String)).void }
  def self.sentry_monitor_check_ins(monitor_config:, slug: nil)
    slug ||= T.must(name).gsub("::", "-").downcase
    sentry_slug = slug[-50..] || slug

    around_perform do |_job, block|
      check_in_id = Sentry.capture_check_in(sentry_slug, :in_progress, monitor_config:)
      start = Sentry.utc_now.to_i

      begin
        block.call

        duration = Sentry.utc_now.to_i - start
        Sentry.capture_check_in(sentry_slug, :ok, check_in_id:, duration:, monitor_config:)
      rescue StandardError
        duration = Sentry.utc_now.to_i - start
        Sentry.capture_check_in(sentry_slug, :error, check_in_id:, duration:, monitor_config:)

        raise
      end
    end
  end