[delayed_job] integration #444

pawelchcki · 2018-06-05T13:36:51Z

Service name: delayed_job
Resource: job.name - this can be a class name or a formatted class + method name, depending on how the job was created
Tags:

delayed_job.id - internal id of a job
delayed_job.queue - queue name if it was set
delayed_job.priority - configured priority of the job
delayed_job.attempts - number of retries that has been attempted

Documentation

The DelayedJob integration uses lifecycle hooks to trace the job executions.

You can enable it through Datadog.configure:

require 'ddtrace'

Datadog.configure do |c|
  c.use :delayed_job, options
end

Where options is an optional Hash that accepts the following parameters:

Key	Description	Default
`service_name`	Service name used for `DelayedJob` instrumentation	delayed_job
`tracer`	A `Datadog::Tracer` instance used to instrument the application. Usually you don't need to set that.	`Datadog.tracer`

delner

Good start. Some points regarding configuration, test state, and a few other minor points.

delner · 2018-06-26T19:49:35Z

spec/ddtrace/contrib/delayed_job/app.rb

@@ -0,0 +1,34 @@
+require 'active_record'
+require 'delayed_job'
+require 'delayed_job_active_record'


Is some datasource required to run delayed_job? And is this why we have delayed_job_active_record?

Yes, in delayed_job the datasource needs to be plugged in. The delayed_job gem is a executor with some interface definitions for the datasources.

delner · 2018-06-26T19:57:59Z

lib/ddtrace/contrib/delayed_job/plugin.rb

+      # DelayedJob plugin that instruments invoke_job hook
+      class Plugin < Delayed::Plugin
+        def self.instrument(job, &block)
+          pin = Pin.get_from(::Delayed::Worker)


Is the use of Pin required? We often use Pin to handle configuration for disparate objects in the same integration (e.g. multiple databases), but otherwise encourage the use of the configuration directly e.g. Datadog.configuration[:delayed_job][:service_name].

The downside I see to the current setup is that calling use :delayed_job won't update the Pin after its been initialized, making it difficult to reconfigure.

delner · 2018-06-26T19:59:05Z

lib/ddtrace/contrib/delayed_job/plugin.rb

+          pin.tracer.shutdown! if pin && pin.tracer
+        end
+
+        callbacks do |lifecycle|


This is a nice way to hook into the framework!

delner · 2018-06-26T20:01:07Z

spec/ddtrace/contrib/delayed_job/plugin_spec.rb

+  let(:tracer) { ::Datadog::Tracer.new(writer: FauxWriter.new) }
+
+  before do
+    Datadog::Contrib::DelayedJob::Patcher.patch


Why do we call patch directly instead of Datadog.configure { |c| c.use :delayed_job }? It would be best to preserve a consistent configuration API experience, whenever possible.

delner · 2018-06-26T20:09:38Z

spec/ddtrace/contrib/delayed_job/app.rb

+logger = Logger.new(STDOUT)
+logger.level = Logger::INFO
+
+ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')


I know we've done this a lot in previous test files to configure ActiveRecord with Minitest, but we really shouldn't be establishing database connections simply by virtue of calling require 'app'. I would consider wrapping this into a module, function, or RSpec reference that can be optionally composed into specific examples, where necessary.

A big downside is that this currently creates a dependence of some kind of preconfigured database state: if this state changes, it could break tests in an indeterminate number of places across the test suite.

In an ideal world, specs should encapsulate and manage the state they need to run successfully, rather than depending on such globals. If specs need to share some kind of common test setup (as opposed to state: they should never share state), then it makes sense to have this setup in composable module within a separate file like this. It gives the specs greater control over what state gets set, and how/when to set it. And if this state changes, it's easier to track down which examples specifically depend upon it.

Also, 💯 for use of in-memory SQLite, rather than depending on an external database service.

Agreed requrie 'app' is not the best approach.

I've switched to using RSpec around to setup and close the DB for each test.

delner · 2018-06-26T20:16:48Z

spec/ddtrace/contrib/delayed_job/plugin_spec.rb

+  describe 'instrumenting worker execution' do
+    let(:worker) { double(:worker, name: 'worker') }
+    before do
+      allow(tracer).to receive(:shutdown!).and_call_original


Is this not a no-op? allow is a stub that does no assertions, and using and_call_original, its just passing through like it was never there.

If you'd like to assert that shutdown! is called, it should be changed to expect(tracer). If you want it to simply skip side effects from #shutdown!, then you can remove and_call_original.

Below I check if shutdown! was called at the right moment. i.e. after the executor actually finished executing.

dd-trace-rb/spec/ddtrace/contrib/delayed_job/plugin_spec.rb

Line 31 in a9fc131

expect(tracer).not_to have_received(:shutdown!)

Interesting, I've never seen have_received before, but that's nice.

But the question still remains, why stub with a no-op just to override with an expectation below? Is it just this implementation of a spy?

Its an implementation of the spy you linked. I use it verify that the shutdown is called in correct order:

worker do execute do end shutdown! end

In first test I verify if execute callback is called.

Then I verify that in execute callback the shutdown! wasn't called, but it is called after the execute finishes.

Okay this is fine. Just wanted to make sure this wasn't an unnecessary stub, but it appears to be part of the spy implementation of have_received.

pawelchcki · 2018-06-28T15:58:53Z

Thanks @delner for review! Code is ready for another round.

delner

We're missing documentation, and I think we still need to improve on how the migrations run in the tests.

delner · 2018-06-28T19:10:16Z

spec/ddtrace/contrib/delayed_job/active_record_setup.rb

@@ -0,0 +1,24 @@
+RSpec.configure do |c|


This is an improvement from a syntax perspective, but it now auto-runs around every example in the test suite. Normally we only run one integration at a time in our CI, but we shouldn't operate on that kind of assumption, as it may not always be true. This should only run around the examples it needs to, when the example explicitly configures itself to do so. (Not by virtue of require.)

delner · 2018-06-28T19:11:48Z

lib/ddtrace/contrib/delayed_job/patcher.rb

+            klass.plugins << Plugin
+          end
+
+          def add_pin(klass)


We're driving out configuration off the configuration API, which is good, but why are we adding a pin here?

delner · 2018-06-28T19:12:27Z

spec/ddtrace/contrib/delayed_job/patcher_spec.rb

+require 'ddtrace'
+require_relative 'active_record_setup'
+
+RSpec.describe Datadog::Contrib::DelayedJob::Patcher do


These tests looks really clean; nice work!

delner · 2018-06-28T19:13:46Z

docs/GettingStarted.md

@@ -952,6 +952,27 @@ Where `options` is an optional `Hash` that accepts the following parameters:
 | --- | --- | --- |
 | ``service_name`` | Service name used for `sidekiq` instrumentation | sidekiq |

+### DelayedJob


We're missing important documentation; this section needs to be added to the list of links above, and the table of supported integrations before that, with appropriate details and links to resources wherever required.

Looks like this has since been added; thanks!

delner · 2018-06-29T15:23:12Z

spec/ddtrace/contrib/delayed_job/patcher_spec.rb

+require 'ddtrace'
+require_relative 'active_record_setup'
+
+RSpec.describe Datadog::Contrib::DelayedJob::Patcher, :delayed_job_active_record do


I dig the use of the trait here to drive test configuration, but I think we still need to update the active_record_setup.rb file to match, no?

delner · 2018-06-29T20:00:23Z

spec/ddtrace/contrib/delayed_job/delayed_job_active_record.rb

@@ -0,0 +1,24 @@
+RSpec.configure do |c|
+  c.around do |example|


By virtue of being required, this is still running around every example in the test suite. This should be changed so it only runs around specific examples when explicitly composed into the example itself (not just by use of require.) Similarly, any specs that require this file, but do not explicitly compose it into its examples should not perform these migrations.

delner · 2018-07-11T20:47:49Z

lib/ddtrace/contrib/delayed_job/patcher.rb

+
+          private
+
+          def unpatch


Consider that this patch is not actually "unpatchable". We should not communicate the expectation that you can "undo" patches, or that you should be able to "undo" patches, since modifying classes and constants is generally irreversible. Trying to support this would be useful for tests, but only for tests; if this kind of feature only needs to exist for tests, then in should exist in the test suite, not in our patcher API.

delner · 2018-07-11T20:49:40Z

spec/ddtrace/contrib/delayed_job/delayed_job_active_record.rb

@@ -0,0 +1,24 @@
+RSpec.configure do |c|
+  c.around(:example, :delayed_job_active_record) do |example|


Like the use of a trait here. 👍

delner · 2018-07-11T20:52:22Z

spec/ddtrace/contrib/delayed_job/plugin_spec.rb

+require 'ddtrace/contrib/delayed_job/plugin'
+require_relative 'delayed_job_active_record'
+
+SampleJob = Struct.new('SampleJob') { def perform; end }


This should be put into an RSpec constant stub, something like:

let(:job_class) { stub_const('SampleJob', Struct.new('SampleJob') { def perform; end }) }

delner

Looks pretty solid, just a couple of smaller things with unpatch and constant stubs.

delner

Looks great. Nice work!

delner · 2018-07-16T16:14:22Z

spec/ddtrace/contrib/delayed_job/plugin_spec.rb

+
+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) }


pawelchcki · 2018-07-16T16:46:25Z

Thanks for review @delner!

pawelchcki added the do-not-merge/WIP Not ready for merge label Jun 5, 2018

pawelchcki self-assigned this Jun 5, 2018

pawelchcki added 7 commits June 11, 2018 14:33

Delayed Job initial implementation

f1f4519

Working deplayed_job tracing

78ffd20

Base for delayed_job tests

064a060

cleanup unnecessary trace setup code

b02ec5c

Update rakefile for delayed_job

6ed0c48

Add all necessary appraisals

6aa98cb

add delayed_job patcher tests

c084b13

pawelchcki force-pushed the feature/add_delayed_job_integration branch from 8fa35d3 to c084b13 Compare June 11, 2018 13:58

pawelchcki added 7 commits June 11, 2018 17:02

Plugin spec

0162b04

add delayed job specs

eca5ffc

Rename test suite

e48c1d4

Add class level comment in DelayedJob::Plugin

792b9d4

updated GettingStarted

dc7156b

cleanup

b6a05a7

fix tests running in 1.9.3

767d697

pawelchcki added integrations Involves tracing integrations feature Involves a product feature and removed do-not-merge/WIP Not ready for merge labels Jun 12, 2018

pawelchcki added this to the 0.13.0 milestone Jun 12, 2018

pawelchcki requested a review from delner June 12, 2018 13:19

pawelchcki assigned delner and unassigned pawelchcki Jun 12, 2018

delner modified the milestones: 0.13.0, 0.14.0 Jun 15, 2018

pawelchcki changed the base branch from 0.13-dev to 0.14-dev June 21, 2018 10:44

Flush on execution end + tests

a9fc131

delner requested changes Jun 26, 2018

View reviewed changes

delner mentioned this pull request Jun 27, 2018

Support for Delayed::Job tracking #393

Closed

pawelchcki added 4 commits June 28, 2018 14:18

use configuration instead of pin

f9898f8

remove useless comment

a2f678e

cleanup app initialization since we're using SqLite

2e611dd

use rspec helper to setup active_record and schema

466a79a

delner requested changes Jun 28, 2018

View reviewed changes

pawelchcki added 2 commits June 29, 2018 14:37

Remove pin from delayedjob worker

9073be2

Add delayed job links

a78f501

delner reviewed Jun 29, 2018

View reviewed changes

Rename active_Record_setup to delayed_job_active_record

b56c1c7

delner requested changes Jun 29, 2018

View reviewed changes

add missing specification

4ea9e6d

delner assigned pawelchcki and unassigned delner Jul 11, 2018

fix tests

f80fe76

delner reviewed Jul 11, 2018

View reviewed changes

delner requested changes Jul 11, 2018

View reviewed changes

pawelchcki added 2 commits July 16, 2018 10:36

use only mocked classes to test delayed_job

ff17388

remove unpatch methods

48e1b79

delner approved these changes Jul 16, 2018

View reviewed changes

pawelchcki merged commit 20a8bfc into 0.14-dev Jul 16, 2018

pawelchcki deleted the feature/add_delayed_job_integration branch July 16, 2018 16:45

sauloperez mentioned this pull request Dec 3, 2018

App monitoring openfoodfoundation/ofn-install#274

Closed

sauloperez mentioned this pull request Jan 2, 2019

Background jobs monitoring openfoodfoundation/ofn-install#318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[delayed_job] integration #444

[delayed_job] integration #444

pawelchcki commented Jun 5, 2018 •

edited

Loading

delner left a comment

delner Jun 26, 2018

pawelchcki Jun 28, 2018

delner Jun 26, 2018

delner Jun 26, 2018

delner Jun 26, 2018

delner Jun 26, 2018

delner Jun 26, 2018

pawelchcki Jun 28, 2018

delner Jun 26, 2018

pawelchcki Jun 28, 2018

pawelchcki Jun 28, 2018 •

edited

Loading

delner Jun 28, 2018

pawelchcki Jun 28, 2018

delner Jun 28, 2018

pawelchcki commented Jun 28, 2018

delner left a comment

delner Jun 28, 2018

delner Jun 28, 2018

delner Jun 28, 2018

delner Jun 28, 2018

delner Jun 29, 2018

delner Jun 29, 2018

pawelchcki Jun 29, 2018

delner Jun 29, 2018

delner Jul 11, 2018

pawelchcki Jul 16, 2018

delner Jul 11, 2018

delner Jul 11, 2018

delner left a comment

delner left a comment

delner Jul 16, 2018

pawelchcki commented Jul 16, 2018

		@@ -0,0 +1,24 @@
		RSpec.configure do \|c\|
		c.around(:example, :delayed_job_active_record) do \|example\|

[delayed_job] integration #444

[delayed_job] integration #444

Conversation

pawelchcki commented Jun 5, 2018 • edited Loading

Documentation

delner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelchcki Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelchcki commented Jun 28, 2018

delner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner left a comment

Choose a reason for hiding this comment

delner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelchcki commented Jul 16, 2018

pawelchcki commented Jun 5, 2018 •

edited

Loading

pawelchcki Jun 28, 2018 •

edited

Loading