Add standalone Rails ActiveJob integration #1639

bensheldon · 2021-08-06T22:42:40Z

Adds a standalone ActiveJob integration that is based on the existing ActiveRecord integration that uses the ActiveSupport::Notifications::Event integration. Connects to #1633.

This instruments the perform.active_job event. I wanted to open this for feedback before instrumenting additional events.

marcotc

Awesome work, @bensheldon!

I think this is definitely on the right track and it would be great if you had the bandwidth to bring it to the finish line.

All the changes you have already in the PR are solid and very clean. 🙇

Given I believe there's value in continuing the work, I'll leave a few comments on how to bring this ActiveJob instrumentation to release:

Because active_job is hard dependency for Rails, we can automatically enable it when Rails instrumentation is enabled. To to this, we should activate it alongside our other sub-Rails instrumentations here:

dd-trace-rb/lib/ddtrace/contrib/rails/framework.rb

Lines 47 to 51 in f7431ce

    
           activate_action_cable!(datadog_config, rails_config) 
        
           activate_active_support!(datadog_config, rails_config) 
        
           activate_action_pack!(datadog_config, rails_config) 
        
           activate_action_view!(datadog_config, rails_config) 
        
           activate_active_record!(datadog_config, rails_config)

Add active_job to the global instrumentation list.
Document this new integration. You can see how the documentation for the qless gem was added, as an example.

marcotc · 2021-08-10T23:00:41Z

spec/ddtrace/contrib/rails/rails_active_job_spec.rb

+      expect(span.resource).to eq('EmptyJob')
+      expect(span.get_tag('active_job.adapter')).to eq('ActiveJob::QueueAdapters::InlineAdapter')
+      expect(span.get_tag('active_job.job.id')).to match(/[0-9a-f\-]{32}/)
+      expect(span.get_tag('active_job.job.queue')).to eq('elephants')


spec/ddtrace/contrib/rails/rails_active_job_spec.rb

marcotc · 2021-08-10T23:03:04Z

spec/ddtrace/contrib/rails/rails_active_job_spec.rb

-      stub_const('EmptyJob', Class.new(ActiveJob::Base) do
-        def perform; end
-      end)
+  context 'with Sidekiq instrumentation' do


What happens when Sidekiq+active_job are both enabled? Do we have both Sidekiq and active_job spans in the same trace?

It's not an issue if we have both, but it would be good to have a test case covering that both appear together in the tracer if that's the case.

If not, then no action is needed.

When both Sidekiq+active_job are enabled active_job, it does result in 2 spans. I will add tests for that.

bensheldon · 2021-08-11T13:06:14Z

@marcotc thank you for the feedback! I will:

Automatically enable it when Rails instrumentation is enabled.
Add to the global instrumentation list
Add documentation
Add a test for when both sidekiq and active_job instrumentation are enabled
Add instrumentation for enqueue

bensheldon · 2021-08-11T17:07:31Z

@marcotc I've made the updates. Could you please do a review of this PR? Thanks!

marcotc

Thank you again, @bensheldon, all changes look great!

One one I forgot to address, which you requested, are the possible other events that emitted by ActiveJob: https://guides.rubyonrails.org/active_support_instrumentation.html#active-job

I think it makes sense to instrument theses as well:
enqueue_at.active_job, enqueue_retry.active_job, retry_stopped.active_job, discard.active_job.

There's no need to write a combination of Sidekiq/ActiveJob for these: a simple direct test each event is enough here. If some of these event naturally overlap in the test setting, you can also merge their case cases and assert multiple spans in one job, if it saves you time.

The only event that I don't think we need is perform_start.active_job, given that perform.active_job is instrumented and seems to organically cover peform_start.

bensheldon · 2021-08-12T00:18:52Z

Fantastic! I will instrument and test:

enqueue_at.active_job
enqueue_retry.active_job
retry_stopped.active_job
discard.active_job

marcotc · 2021-08-13T18:41:05Z

@bensheldon, not sure if you know this, but you can run docker-compose run --rm tracer-2.2 in ddtrace's root directory to get a Ruby 2.2 environment for testing (along with all the required infrastructure [e.g databases]).

You can then run the same commands as CI there (example):

bundle install
bundle exec appraisal install
bundle exec appraisal ruby-2.2.10-rails4-postgres-sidekiq rake spec:railsactivejob

bensheldon · 2021-08-13T18:43:52Z

@marcotc thanks for the help! Sorry about all the failed CI emails you're probably getting 😄 Quite the test matrix ♾️

marcotc · 2021-08-13T19:03:58Z

@marcotc thanks for the help! Sorry about all the failed CI emails you're probably getting 😄 Quite the test matrix ♾️

No concern at all with that!

I just want to make sure you have the fastest tool for feedback available.

bensheldon · 2021-08-13T19:51:41Z

@marcotc I've instrumented all the events and tests are all passing. Could you give this another Review? Thanks!

marcotc

Thank you so much, @bensheldon!
This is integration super well flushed out!
🙇

We'll let you know when the next release goes out (which will include your changes).

marcotc · 2021-10-06T21:39:44Z

@bensheldon, thank you again so, so much for your contribution. 0.53.0 was just released, including the changes in this PR!

bensheldon requested a review from a team August 6, 2021 22:42

bensheldon force-pushed the active_job_integration branch 3 times, most recently from 5763e2d to 6a21e43 Compare August 7, 2021 00:00

bensheldon mentioned this pull request Aug 7, 2021

Add Generic Rails ActiveJob integration #1633

Closed

marcotc reviewed Aug 10, 2021

View reviewed changes

marcotc self-assigned this Aug 10, 2021

marcotc added community Was opened by a community member integrations Involves tracing integrations labels Aug 10, 2021

bensheldon force-pushed the active_job_integration branch 7 times, most recently from f4e67db to f16f4b9 Compare August 11, 2021 15:16

marcotc reviewed Aug 11, 2021

View reviewed changes

bensheldon force-pushed the active_job_integration branch 9 times, most recently from 4bae1cb to 84e1e45 Compare August 13, 2021 18:34

bensheldon force-pushed the active_job_integration branch from 84e1e45 to 4073772 Compare August 13, 2021 19:18

Add standalone Rails ActiveJob integration

599ecc7

bensheldon force-pushed the active_job_integration branch from 4073772 to 599ecc7 Compare August 13, 2021 19:41

marcotc approved these changes Aug 13, 2021

View reviewed changes

marcotc merged commit 80fb6e6 into DataDog:master Aug 13, 2021

github-actions bot added this to the 0.53.0 milestone Aug 13, 2021

agrobbin mentioned this pull request Sep 20, 2021

Correlate Active Job logs to the active DataDog trace #1694

Merged

bensheldon mentioned this pull request Oct 30, 2023

feat!(active_job): Use ActiveSupport instead of patches open-telemetry/opentelemetry-ruby-contrib#677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add standalone Rails ActiveJob integration #1639

Add standalone Rails ActiveJob integration #1639

bensheldon commented Aug 6, 2021

marcotc left a comment

marcotc Aug 10, 2021

marcotc Aug 10, 2021

bensheldon Aug 11, 2021

bensheldon commented Aug 11, 2021 •

edited

Loading

bensheldon commented Aug 11, 2021

marcotc left a comment

bensheldon commented Aug 12, 2021 •

edited

Loading

marcotc commented Aug 13, 2021 •

edited

Loading

bensheldon commented Aug 13, 2021

marcotc commented Aug 13, 2021

bensheldon commented Aug 13, 2021

marcotc left a comment

marcotc commented Oct 6, 2021

	activate_action_cable!(datadog_config, rails_config)
	activate_active_support!(datadog_config, rails_config)
	activate_action_pack!(datadog_config, rails_config)
	activate_action_view!(datadog_config, rails_config)
	activate_active_record!(datadog_config, rails_config)

Add standalone Rails ActiveJob integration #1639

Add standalone Rails ActiveJob integration #1639

Conversation

bensheldon commented Aug 6, 2021

marcotc left a comment

Choose a reason for hiding this comment

marcotc Aug 10, 2021

Choose a reason for hiding this comment

marcotc Aug 10, 2021

Choose a reason for hiding this comment

bensheldon Aug 11, 2021

Choose a reason for hiding this comment

bensheldon commented Aug 11, 2021 • edited Loading

bensheldon commented Aug 11, 2021

marcotc left a comment

Choose a reason for hiding this comment

bensheldon commented Aug 12, 2021 • edited Loading

marcotc commented Aug 13, 2021 • edited Loading

bensheldon commented Aug 13, 2021

marcotc commented Aug 13, 2021

bensheldon commented Aug 13, 2021

marcotc left a comment

Choose a reason for hiding this comment

marcotc commented Oct 6, 2021

bensheldon commented Aug 11, 2021 •

edited

Loading

bensheldon commented Aug 12, 2021 •

edited

Loading

marcotc commented Aug 13, 2021 •

edited

Loading