scoutapp / scout_apm_ruby

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

Feature Request: Add ActiveJob Support #201

Open jtcontreras90 opened 6 years ago

jtcontreras90 commented 6 years ago

There should be Rails' pure ActiveJob support for different queue adapters based in it. In particular, I had a lot of trouble configuring this gem with active_elastic_job. Finally, I configured my own instrument like this (based in the gist avaliable in documentation under Sneakers section)

mathieujobin commented 5 years ago

Thanks for this @jtcontreras90

I really need this myself as well

does it work for you OK?

Couldn't we not call it once on a Active::Job monkey patch, instead of modifying every job files?

jtcontreras90 commented 5 years ago

You are welcome @mathieujobin

It does work fine for me and it has been working fine on production environment for more than 10 months.

The thing about the comment on the gist is that is not entirely honest. I did not include the instrument on every job file. Instead, I created an initializer named scout.rb with the following code:

if Rails.application.config.active_job.queue_adapter == :active_elastic_job
  Rails.application.config.after_initialize do
    ActiveJob::Base.descendants.each do |subclass|
      subclass.include(ScoutApm::BackgroundJobIntegrations::ActiveElasticJob::Instruments)
    end
  end
end
ioquatix commented 5 years ago

@cschneid @dlanderson would you like me to implement this?

cschneid commented 5 years ago

I thought I wrote a piece about ActiveJob in another issue, but I can't locate it now. The gist of it is:

ActiveJob doesn't give us the information needed to generically instrument jobs. It's a really thin wrapper that makes nearly no assumptions about the data being sent to the job runner. Which means that even basic things like extracting the name of the job being requested aren't possible to extract in a general way.

In addition, it also doesn't specify execution style (correctly, but inconvenient for us), so delayed-job runs in thread, sidekiq creates a new threads and resque forks. Those all require their own handling to be sure the data is captured correctly, and can't be totally generic.

I can take another look at it in a while, but I'm pretty sure totally generic AJ support isn't possible with how the Scout agent works.

ioquatix commented 5 years ago

That makes sense.

If that's the case, why don't I raise the issue with the Rails team and see if we can get the appropriate hooks for instrumentation?

cschneid commented 5 years ago

That would be fine - we've mostly not looked into instrumentation hooks because we have to support back to before the hooks existed, so we need a specialized monkey patch approach anyway. But if you want to look into what hooks exist, and if new ones should be implemented, that would be excellent.

ioquatix commented 5 years ago

I will have limited time until after RubyKaigi, but later this month I'll dig into this issue and see how we can move forward.

mathieujobin commented 5 years ago

Exactly as suggested by @jtcontreras90 I deployed this to production today and it works magically

https://gist.github.com/mathieujobin/2a705d7820f9f84bbcebdfc495872b38