Delayed::Job job name improvement to support ActiveJob #605
Conversation
There was a problem hiding this comment.
Thanks for submitting this! It's always great to see contributions like this to improve the tracer :)
I'll do a more thorough review of this a bit later, but wanted to leave a few thoughts to keep the ball rolling for now. Looking forward to digging deeper into this!
| @@ -7,13 +7,22 @@ | |||
| require_relative 'delayed_job_active_record' | |||
|
|
|||
| RSpec.describe Datadog::Contrib::DelayedJob::Plugin, :delayed_job_active_record do | |||
| let(:sample_job_object) { double('sample_job', perform: nil) } | |||
| let(:sample_job_class) { class_double('SampleJob', new: sample_job_object) } | |||
| class SampleJob | |||
There was a problem hiding this comment.
I don't think we should replace these let blocks with constants. The constants are immutable between tests and can cause naming conflicts. I suspect the existing RSpec doubles might not cut it because the changes introduced might actually depend on reflect real constants, however, I think we could convert these blocks to dynamic constants instead which would cover both sets of needs.
Something like:
let(:sample_job_object) do
stub_const("SampleJob", Class.new do
def perform; end
end)
end| @@ -9,7 +9,14 @@ class Plugin < Delayed::Plugin | |||
| def self.instrument(job, &block) | |||
| return block.call(job) unless tracer && tracer.enabled | |||
|
|
|||
| tracer.trace(Ext::SPAN_JOB, service: configuration[:service_name], resource: job.name) do |span| | |||
| # If DelayedJob is used through ActiveJob, `job.name` is always "ActiveJob::QueueAdapters::DelayedJobAdapter::JobWrapper" | |||
| if job.payload_object.respond_to?(:job_data) | |||
There was a problem hiding this comment.
The existence of this job_data method appears to be the key here to support ActiveJob.
Can you explain the differences between vanilla DelayedJobs and ones that come through ActiveJob? Specifically, what job is and how its configured? And could you also tell me a little more about the conditions when you expect each of these branches to be executed?
My thoughts behind those questions are that it would be best to avoid any specific configuration for ActiveJob in the DelayedJob integration, since it would introduce a kind of coupling via inversion of dependencies. However, what we have here does look pretty general purpose (which is promising); I'd just like to confirm it's not some kind of defacto coupling.
There was a problem hiding this comment.
ActiveJob is another abstraction, it’s the public API in front of any background jobs library. DelayedJob is one backend that executes the code that ActiveJob gives it. Rails uses ActiveJob to support a variety of queueing backends and DelayedJob is just one of them.
I totally understand how creating an ActiveJob specific configuration creates an awkward inversion of dependencies. Knowing about the implementation details of ActiveJob isn't great.
I figured I’d dive down this avenue to see if y’all were amenable to supporting two different job payload objects. This solution already exists for Airbrake for error catching, which is a good indicator that it's not too terrible of an idea — at least there's precedent for this kind of solution in other tools.
Given that ActiveJob is the default framework for Rails 5+, I think it would be great to get this library patched up to work nicely with it for those folks (myself included) that are using ActiveJob with DelayedJob.
There was a problem hiding this comment.
I see, yeah. We're definitely wanting to add ActiveJob support to the library here in the future, and getting this working with DelayedJob would great.
Rails stuff can be a bit demanding of such special treatment, so I think I can appreciate that the change is at least fairly straightforward despite the introduction of coupling. I lean towards accepting it, but just wanted to make sure we're asking and answering the question "is this as simple and durable as it can be?"
There was a problem hiding this comment.
I'm confident this is the simplest solution. job will always respond to payload_object because that's how DJ works, so if job_data is a key in payload_object we can be confident that it's an ActiveJob job and take the job_class value from the payload. What do you think?
There was a problem hiding this comment.
I think this can work as is. If we discover some better way of doing this in the future we can always change it, but for now it's good that we're getting some improved behavior out of this.
delner
added
integrations
|
@agirlnamedsophia Can you take a look at the build errors? (Rubocop issues?) And then rebase this on the latest master? If we can do that soon, I think we can get this in the next bugfix release (today or tomorrow), else we'll have to move it to the release after that. |
|
Awesome! @delner I'm unsure what the other failures are, but it's been rebased on master and rubocop seems happy (I think)! |
|
@agirlnamedsophia Yeah I see that build error, too. It just appeared on another PR, so I don't think its because of your changes. In all likelihood, someone released a new version of |
|
@agirlnamedsophia We found and fixed the source of the problem; just need CI to rebuild using version If you're able to rebase/force push to change the commit SHAs (or create a new commit), it should hopefully bust the CI gem cache and fix the build. |
agirlnamedsophia
force-pushed
the
sr/master/send-delayed-job-job-names
branch
from
b35e1e9
to
0c7b262
Compare
|
@delner everything seems to be working except the prerelease Gem job and maybe that's because I'm on a fork? |
|
@agirlnamedsophia Yup that's why; this is as good as a passing build. |
There was a problem hiding this comment.
Looks great! Thanks for your contribution @agirlnamedsophia ! 💯
|
thank you for accepting this suggestion! 🙌 |
My team and I are excited to use this library (and DataDog itself) and are happy with so much automatic instrumentation and tagging that comes with it. However we noticed in using this library that all of our delayed jobs were showing up as
JobWrapperinstead of the specific job's name, because
ActiveJobincludes another abstraction around the job class itself.Before, everything was showing up under the one item at the top. Now you can see the individual jobs populating with their specific names.
We also included tests that are slightly less stubby so as to avoid adding a dependency on
ActiveJob, but mimic what ActiveJob would send in real life.