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

Add standalone Rails ActiveJob integration #1639

Merged
merged 1 commit into from
Aug 13, 2021

Conversation

bensheldon
Copy link
Contributor

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.

@bensheldon bensheldon requested a review from a team August 6, 2021 22:42
@bensheldon bensheldon force-pushed the active_job_integration branch 3 times, most recently from 5763e2d to 6a21e43 Compare August 7, 2021 00:00
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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:
    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)
  2. Add active_job to the global instrumentation list.
  3. Document this new integration. You can see how the documentation for the qless gem was added, as an example.

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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2021-08-10 at 4 00 17 PM

spec/ddtrace/contrib/rails/rails_active_job_spec.rb Outdated Show resolved Hide resolved
stub_const('EmptyJob', Class.new(ActiveJob::Base) do
def perform; end
end)
context 'with Sidekiq instrumentation' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@marcotc marcotc self-assigned this Aug 10, 2021
@marcotc marcotc added community Was opened by a community member integrations Involves tracing integrations labels Aug 10, 2021
@bensheldon
Copy link
Contributor Author

bensheldon commented Aug 11, 2021

@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 bensheldon force-pushed the active_job_integration branch 7 times, most recently from f4e67db to f16f4b9 Compare August 11, 2021 15:16
@bensheldon
Copy link
Contributor Author

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

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

bensheldon commented Aug 12, 2021

Fantastic! I will instrument and test:

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

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

marcotc commented Aug 13, 2021

@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
Copy link
Contributor Author

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

@marcotc
Copy link
Member

marcotc commented Aug 13, 2021

@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
Copy link
Contributor Author

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

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marcotc merged commit 80fb6e6 into DataDog:master Aug 13, 2021
@github-actions github-actions bot added this to the 0.53.0 milestone Aug 13, 2021
@marcotc
Copy link
Member

marcotc commented Oct 6, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants