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

Sidekiq client-side tracing #602

Merged
merged 12 commits into from
Nov 26, 2018
35 changes: 35 additions & 0 deletions lib/ddtrace/contrib/sidekiq/client_tracer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
require 'ddtrace/contrib/sidekiq/tracing'

module Datadog
module Contrib
module Sidekiq
# Tracer is a Sidekiq client-side middleware which traces job enqueues/pushes
class ClientTracer
include Tracing

def initialize(options = {})
super
@sidekiq_service = options[:client_service_name] || Datadog.configuration[:sidekiq][:client_service_name]
end

# Client middleware arguments are documented here:
# https://github.com/mperham/sidekiq/wiki/Middleware#client-middleware
def call(worker_class, job, queue, redis_pool)
service = @sidekiq_service
set_service_info(service)

resource = job_resource(job)

@tracer.trace(Ext::SPAN_PUSH, service: service) do |span|
span.resource = resource
span.set_tag(Ext::TAG_JOB_ID, job['jid'])
span.set_tag(Ext::TAG_JOB_QUEUE, job['queue'])
span.set_tag(Ext::TAG_JOB_WRAPPER, job['class']) if job['wrapped']

yield
end
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/ddtrace/contrib/sidekiq/configuration/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ module Configuration
# Custom settings for the Sidekiq integration
class Settings < Contrib::Configuration::Settings
option :service_name, default: Ext::SERVICE_NAME
option :client_service_name, default: Ext::CLIENT_SERVICE_NAME
end
end
end
Expand Down
2 changes: 2 additions & 0 deletions lib/ddtrace/contrib/sidekiq/ext.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ module Sidekiq
module Ext
APP = 'sidekiq'.freeze
SERVICE_NAME = 'sidekiq'.freeze
CLIENT_SERVICE_NAME = 'sidekiq-client'.freeze

SPAN_PUSH = 'sidekiq.push'.freeze
SPAN_JOB = 'sidekiq.job'.freeze

TAG_JOB_DELAY = 'sidekiq.job.delay'.freeze
Expand Down
11 changes: 9 additions & 2 deletions lib/ddtrace/contrib/sidekiq/patcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,17 @@ def patched?
def patch
do_once(:sidekiq) do
begin
require 'ddtrace/contrib/sidekiq/tracer'
require 'ddtrace/contrib/sidekiq/client_tracer'
::Sidekiq.configure_client do |config|
config.client_middleware do |chain|
chain.add(Sidekiq::ClientTracer)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

end
end

require 'ddtrace/contrib/sidekiq/server_tracer'
::Sidekiq.configure_server do |config|
config.server_middleware do |chain|
chain.add(Sidekiq::Tracer)
chain.add(Sidekiq::ServerTracer)
end
end
rescue StandardError => e
Expand Down
50 changes: 50 additions & 0 deletions lib/ddtrace/contrib/sidekiq/server_tracer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
require 'ddtrace/contrib/sidekiq/tracing'

module Datadog
module Contrib
module Sidekiq
# Tracer is a Sidekiq server-side middleware which traces executed jobs
class ServerTracer
include Tracing

def initialize(options = {})
super
@sidekiq_service = options[:service_name] || Datadog.configuration[:sidekiq][:service_name]
end

def call(worker, job, queue)
resource = job_resource(job)

service = service_from_worker_config(resource) || @sidekiq_service
set_service_info(service)

@tracer.trace(Ext::SPAN_JOB, service: service, span_type: Datadog::Ext::AppTypes::WORKER) do |span|
span.resource = resource
span.set_tag(Ext::TAG_JOB_ID, job['jid'])
span.set_tag(Ext::TAG_JOB_RETRY, job['retry'])
span.set_tag(Ext::TAG_JOB_QUEUE, job['queue'])
span.set_tag(Ext::TAG_JOB_WRAPPER, job['class']) if job['wrapped']
span.set_tag(Ext::TAG_JOB_DELAY, 1000.0 * (Time.now.utc.to_f - job['enqueued_at'].to_f))

yield
end
end

private

def service_from_worker_config(resource)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self (and out of scope for this PR): we should remove this. We don't want jobs defining tracer functions on themselves; this should be handled in our integration configuration (probably by leveraging the new multiplexing feature.)

As this is apparently existing behavior from the previous file (before you refactored this), this isn't a blocker. We'll do this in a future PR.

# Try to get the Ruby class from the resource name.
worker_klass = begin
Object.const_get(resource)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also old code but... can you think of any scenarios where the resource (i.e. class name) would not exist in the namespace?

If failing to resolve a constant and raising an error is a very common case, then this could be incurring a significant performance cost. If that is true, we'd want to fix this (now or at some point in the future.) Otherwise if this is just an edge case, and a safety of sorts, then I don't think we need to worry about this.

This isn't a blocker for this PR most likely, since it's existing behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@delner: This is adapted from the original tracer implementation, but given your comment above I think this seems like a good enough time as any to remove support for workers defining a .datadog_tracer_config method (and therefore not need this Object.const_get). What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think we'll want to remove this too, but to do so, we'll have to deprecate the feature and remove it in a future release. I've added an item to our backlog to do each of these things in future PRs.

rescue NameError
nil
end

if worker_klass.respond_to?(:datadog_tracer_config)
worker_klass.datadog_tracer_config[:service_name]
end
end
end
end
end
end
72 changes: 0 additions & 72 deletions lib/ddtrace/contrib/sidekiq/tracer.rb

This file was deleted.

38 changes: 38 additions & 0 deletions lib/ddtrace/contrib/sidekiq/tracing.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
require 'ddtrace/ext/app_types'
require 'ddtrace/contrib/sidekiq/ext'

module Datadog
module Contrib
module Sidekiq
# Common functionality used by both client-side and server-side tracers.
module Tracing
def initialize(options = {})
@tracer = options[:tracer] || Datadog.configuration[:sidekiq][:tracer]
end

protected

# If class is wrapping something else, the interesting resource info
# is the underlying, wrapped class, and not the wrapper. This is
# primarily to support `ActiveJob`.
def job_resource(job)
if job['wrapped']
job['wrapped']
else
job['class']
end
end

def set_service_info(service)
# Ensure the tracer knows about this service.
return if @tracer.services[service]
@tracer.set_service_info(
service,
Ext::APP,
Datadog::Ext::AppTypes::WORKER
)
end
end
end
end
end
2 changes: 1 addition & 1 deletion spec/ddtrace/contrib/rails/support/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

if ENV['USE_SIDEKIQ']
require 'sidekiq/testing'
require 'ddtrace/contrib/sidekiq/tracer'
require 'ddtrace/contrib/sidekiq/server_tracer'
end

RSpec.shared_context 'Rails base application' do
Expand Down
2 changes: 1 addition & 1 deletion spec/ddtrace/contrib/rails/support/rails3.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

if ENV['USE_SIDEKIQ']
require 'sidekiq/testing'
require 'ddtrace/contrib/sidekiq/tracer'
require 'ddtrace/contrib/sidekiq/server_tracer'
end

require 'ddtrace/contrib/rails/support/controllers'
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/rails/support/rails4.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

if ENV['USE_SIDEKIQ']
require 'sidekiq/testing'
require 'ddtrace/contrib/sidekiq/tracer'
require 'ddtrace/contrib/sidekiq/server_tracer'
end

require 'ddtrace/contrib/rails/support/controllers'
Expand Down Expand Up @@ -43,7 +43,7 @@ def config.database_configuration
# add Sidekiq middleware
Sidekiq::Testing.server_middleware do |chain|
chain.add(
Datadog::Contrib::Sidekiq::Tracer
Datadog::Contrib::Sidekiq::ServerTracer
)
end
end
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/rails/support/rails5.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

if ENV['USE_SIDEKIQ']
require 'sidekiq/testing'
require 'ddtrace/contrib/sidekiq/tracer'
require 'ddtrace/contrib/sidekiq/server_tracer'
end

require 'ddtrace/contrib/rails/support/controllers'
Expand Down Expand Up @@ -41,7 +41,7 @@ def config.database_configuration
# add Sidekiq middleware
Sidekiq::Testing.server_middleware do |chain|
chain.add(
Datadog::Contrib::Sidekiq::Tracer
Datadog::Contrib::Sidekiq::ServerTracer
)
end
end
Expand Down
4 changes: 2 additions & 2 deletions test/contrib/rails/apps/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require 'ddtrace'
if ENV['USE_SIDEKIQ']
require 'sidekiq/testing'
require 'ddtrace/contrib/sidekiq/tracer'
require 'ddtrace/contrib/sidekiq/server_tracer'
end

module RailsTrace
Expand All @@ -25,7 +25,7 @@ def initialize(*args)
# add Sidekiq middleware
Sidekiq::Testing.server_middleware do |chain|
chain.add(
Datadog::Contrib::Sidekiq::Tracer
Datadog::Contrib::Sidekiq::ServerTracer
)
end
end
Expand Down
2 changes: 1 addition & 1 deletion test/contrib/rails/rails_active_job_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
require 'helper'
require 'sidekiq/testing'
require 'contrib/rails/test_helper'
require 'ddtrace/contrib/sidekiq/tracer'
require 'ddtrace/contrib/sidekiq/server_tracer'
require 'active_job'

class RailsActiveJobTest < ActionController::TestCase
Expand Down
4 changes: 2 additions & 2 deletions test/contrib/rails/rails_sidekiq_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
require 'helper'
require 'sidekiq/testing'
require 'contrib/rails/test_helper'
require 'ddtrace/contrib/sidekiq/tracer'
require 'ddtrace/contrib/sidekiq/server_tracer'

class RailsSidekiqTest < ActionController::TestCase
setup do
Expand Down Expand Up @@ -43,7 +43,7 @@ def perform; end

# add Sidekiq middleware
Sidekiq::Testing.server_middleware do |chain|
chain.add(Datadog::Contrib::Sidekiq::Tracer, tracer: @tracer, service_name: 'rails-sidekiq')
chain.add(Datadog::Contrib::Sidekiq::ServerTracer, tracer: @tracer, service_name: 'rails-sidekiq')
end

# do something to force middleware execution
Expand Down
59 changes: 59 additions & 0 deletions test/contrib/sidekiq/client_tracer_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
require 'contrib/sidekiq/tracer_test_base'

class ClientTracerTest < TracerTestBase
class EmptyWorker
include Sidekiq::Worker

def perform(); end
end

def setup
super

Sidekiq.configure_client do |config|
config.client_middleware.clear

config.client_middleware do |chain|
chain.add(Datadog::Contrib::Sidekiq::ClientTracer,
tracer: @tracer, enabled: true)
end
end

Sidekiq::Testing.server_middleware.clear
end

def test_empty
@tracer.trace('parent.span', service: 'parent-service') do
EmptyWorker.perform_async
end

spans = @writer.spans
assert_equal(2, spans.length)

parent_span, child_span = spans

assert_equal('parent.span', parent_span.name)
assert_equal(0, parent_span.status)
assert_nil(parent_span.parent)

assert_equal('sidekiq-client', child_span.service)
assert_equal('ClientTracerTest::EmptyWorker', child_span.resource)
assert_equal('default', child_span.get_tag('sidekiq.job.queue'))
assert_equal(0, child_span.status)
assert_equal(parent_span, child_span.parent)
end

def test_empty_parentless
EmptyWorker.perform_async

spans = @writer.spans
assert_equal(1, spans.length)

span = spans.first
assert_equal('sidekiq-client', span.service)
assert_equal('ClientTracerTest::EmptyWorker', span.resource)
assert_equal('default', span.get_tag('sidekiq.job.queue'))
assert_equal(0, span.status)
assert_nil(span.parent)
end
end
Loading