-
Notifications
You must be signed in to change notification settings - Fork 375
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
Changes from all commits
79c2eff
30bb41e
7253b0a
68a3ad1
894fb3c
6a7471e
bae1b48
ffd6cfd
c398e7e
0b1a092
a2fc245
d84dfd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
This file was deleted.
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 |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍