Skip to content

allow resque service name to be configurable #221

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.

Sign up for GitHub

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

Closed
drewbailey wants to merge 1 commit into from
Closed

allow resque service name to be configurable #221

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/ddtrace/contrib/resque/resque_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def after_perform(*args)
end

Resque.before_first_fork do
service = ::Rails.configuration.datadog_trace.fetch(:resque_service) || 'resque'
pin = Datadog::Pin.get_from(Resque)
pin.tracer.set_service_info(pin.service, 'resque', Datadog::Ext::AppTypes::WORKER)
pin.tracer.set_service_info(pin.service, service, Datadog::Ext::AppTypes::WORKER)
Copy link
Contributor

@palazzem palazzem Oct 10, 2017

Choose a reason for hiding this comment

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

Thanks @drewbailey for the PR! Actually the set_service_info only sends metadata to change some UI behavior (i.e. is it a Web, Database or a Cache service?). Our current signature is: https://github.com/drewbailey/dd-trace-rb/blob/6c3ed6302d11432d2985b5a2f65a6e4e04b20cdd/lib/ddtrace/tracer.rb#L128-L139

where:

  • service is your service name, so what you choose with the Pin instance; it's not related to what will be your service name (maybe we should document that better)
  • app is the application name used to distinguish the framework/library. Examples are rails, resque, faraday and so on. Not strictly related to the service itself
  • app_type used to define if it's a worker, cache, web service etc. It's mostly used in the UI to filter services

Said that, if we miss the set_service_info call, nothing really changes in how we collect data. Anyway, can you check if having that in your Rails initializer works or not:

# config/initializers/datadog-tracer.rb

Rails.configuration.datadog_trace = {
  auto_instrument: true,
  # ....
}

# change the Resque service name (may need a require 'resque')
pin = Datadog::Pin.get_from(::Resque)
pin.service = 'resque-queues'

Thank you again for the PR and any feedback you'll provide!

Copy link
Contributor Author

@drewbailey drewbailey Oct 10, 2017

Choose a reason for hiding this comment

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

Thanks for the reply @palazzem! That seems to work although it unfortunately needs to be guarded in the event that pin is nil when the app initializes. Would the following be acceptable?

require 'ddtrace/ext/app_types'
require 'resque'

module Datadog
  module Contrib
    module Resque
      # Uses Resque job hooks to create traces
      module ResqueJob
        def around_perform(*args)
          pin = Pin.get_from(::Resque)
          pin.tracer.trace('resque.job', service: pin.service) do |span|
            span.resource = name
            span.span_type = pin.app_type
            yield
            span.service = pin.service
          end
        end

        def after_perform(*args)
          pin = Pin.get_from(::Resque)
          pin.tracer.shutdown!
        end
      end
    end
  end
end

Resque.before_first_fork do
+ service = ::Rails.configuration.datadog_trace.fetch(:resque_service) || 'resque'
   pin = Datadog::Pin.get_from(Resque)
+ pin.service = service
  pin.tracer.set_service_info(pin.service, service, Datadog::Ext::AppTypes::WORKER)
end

Copy link
Contributor

@palazzem palazzem Oct 11, 2017

Choose a reason for hiding this comment

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

Probably yes, but would be preferable not having any Rails specific configuration in the integration. Mostly we prefer to keep each single integration Rails agnostic, though the current configuration system couples a lot of things (so your PR makes sense and is correct with our current implementation).

Anyway, we're rewriting the configuration system in #203 that allows exactly what you need without changing the resque_service from the Rails config. It would simply:

Datadog.configure do |c|
  c.use :rails, # ... some Rails specific options
  c.use :resque, service_name: 'resque-queues'
end

That PR should be ready very soon. Do you mind if we wait a bit more so we can update your contribution with the new configuration and see how does it look?

Also feel free to give your feedback to the PR above!

Copy link
Contributor Author

@drewbailey drewbailey Oct 11, 2017

Choose a reason for hiding this comment

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

Thanks for the explanation, makes sense! Looking forward to #203

end