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

Adds Resque integration #204

Merged
merged 7 commits into from
Oct 6, 2017
Merged

Adds Resque integration #204

merged 7 commits into from
Oct 6, 2017

Conversation

ryplo
Copy link
Contributor

@ryplo ryplo commented Sep 29, 2017

Overview

Adds the Resque integration, using the built-in hooks. To create a Job that is traced during the execution, just extend our base class:

require 'ddtrace'
require 'ddtrace/contrib/resque/resque_job'

# patch Resque
Datadog::Monkey.patch_module(:resque)

class MyJob
  # extend MyJob with integration hooks
  extend Datadog::Contrib::Resque::ResqueJob
   
  def self.perform(*args)
    # do_something
  end
end

@ryplo ryplo requested review from palazzem and removed request for palazzem September 29, 2017 19:43
@ryplo ryplo added integrations Involves tracing integrations do-not-merge/WIP Not ready for merge labels Sep 29, 2017
@ryplo ryplo changed the title Rachel/adds resque integration Adds Resque integration Sep 29, 2017
@ryplo ryplo force-pushed the rachel/adds-resque-integration branch from 99485fd to aaae4f3 Compare September 29, 2017 23:18
@palazzem palazzem added this to the 0.9.0 milestone Sep 30, 2017
@palazzem palazzem changed the base branch from rachel/implement-shutdown to master October 2, 2017 02:24
@palazzem palazzem self-requested a review October 2, 2017 02:24
@palazzem palazzem force-pushed the rachel/adds-resque-integration branch from aaae4f3 to 628ded1 Compare October 2, 2017 02:36
@ryplo ryplo force-pushed the rachel/adds-resque-integration branch from 628ded1 to 990add8 Compare October 2, 2017 14:25
Datadog::Pin.new(SERVICE, app_type: Ext::AppTypes::WORKER).onto(base)
end

# rubocop:disable Style/RedundantSelf
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't we use simply follow rubocop suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially thought that was needed to call for the base class. I've changed it!

# rubocop:disable Style/RedundantSelf
def around_perform(*args)
pin = self.datadog_pin
pin.tracer.set_service_info(SERVICE, 'resque', Ext::AppTypes::WORKER)
Copy link
Contributor

@palazzem palazzem Oct 2, 2017

Choose a reason for hiding this comment

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

we should not do this in the around_perform, because it means making this calls before each execution. set_service_info call must be done only once per initialization, so it would be great if we can find another hook or class initialization to send the service metadata.

def around_perform(*args)
pin = self.datadog_pin
pin.tracer.set_service_info(SERVICE, 'resque', Ext::AppTypes::WORKER)
pin.tracer.trace('resque.job', service: SERVICE) do |span|
Copy link
Contributor

Choose a reason for hiding this comment

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

the service must be set according to the pin, since users may change the service name later.


def after_perform(*args)
pin = self.datadog_pin
pin.tracer.shutdown!
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

SERVICE = 'resque'.freeze

def self.extended(base)
Datadog::Pin.new(SERVICE, app_type: Ext::AppTypes::WORKER).onto(base)
Copy link
Contributor

Choose a reason for hiding this comment

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

we attach the Pin to the base class, but we don't provide a clean way to change the service name. Can you write here an example of how I change the service name from the main app? Let's say I have Rails + Resque, how do I change the service name from resque to my-app-at-resque from a Rails initializer?

@tracer = enable_test_tracer!
end

def test_successful_job
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a test where we change the service name using the Pin?

@ryplo ryplo force-pushed the rachel/adds-resque-integration branch from 647071e to 990add8 Compare October 2, 2017 22:29
@ryplo ryplo force-pushed the rachel/adds-resque-integration branch from 8df9cfd to b09c73f Compare October 4, 2017 20:50
@palazzem palazzem self-requested a review October 6, 2017 17:01
@palazzem palazzem removed the do-not-merge/WIP Not ready for merge label Oct 6, 2017
Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

Wonderful work @ryplo ! thank you!

@palazzem palazzem merged commit 02caadd into master Oct 6, 2017
@palazzem palazzem deleted the rachel/adds-resque-integration branch October 6, 2017 18:05
@palazzem palazzem self-requested a review October 9, 2017 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants