add sidekiq integration #79
Conversation
galdor
force-pushed
the
nicolas/sidekiq
branch
from
169c26f
to
fd6d2c0
Compare
|
One thing: we should ensure we have the same app_type and span_type as Celery. https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/ext/__init__.py#L6 |
lib/ddtrace/ext/app_types.rb
Outdated
| @@ -4,6 +4,7 @@ module AppTypes | |||
| WEB = 'web'.freeze | |||
| DB = 'db'.freeze | |||
| CACHE = 'cache'.freeze | |||
| JOB_PROCESSOR = 'job_processor'.freeze | |||
There was a problem hiding this comment.
actually we're using worker as suggested in another comment: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/ext/__init__.py#L6
| @tracer.trace('sidekiq.job', | ||
| service: @default_service, span_type: 'job') do |span| | ||
| span.resource = job['class'] | ||
| span.set_tag('sidekiq.job.queue', job['queue']) |
There was a problem hiding this comment.
is this the only tag that we have? can't we retrieve more details about the job like: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/contrib/celery/task.py#L108-L109
We should only add useful tags so if this is what sidekiq provides, we're good.
| To modify the default configuration, simply pass arguments to the middleware. | ||
| For example, to change the default service name and activate the debug mode: | ||
|
|
||
| Sidekiq.configure_server do |config| |
There was a problem hiding this comment.
Actually I'm wondering what happen when we use Sidekiq with Rails. The current status is that if we install both Sidekiq and Rails, when Sidekiq is executed via bundle exec ... the Rails instrumentation is activated and Sidekiq Redis calls are traced (even the x seconds polling). Is it the expected behavior? I think that this middleware must be the only way to instrument the library so we may investigate and remove the side-effect of Rails instrumentation.
Also I was thinking about providing a good pattern (I think docs are enough) to instrument Sidekiq inside the Rails initializer so that our tracing configurations are all together. In this way Sidekiq is still separated from Rails instrumentation but users can easily follow our docs to share the Rails configuration with Sidekiq.
There was a problem hiding this comment.
Sidekiq uses the redis library, so its calls will be traced by the redis contrib. The sidekiq contrib traces job executions, so I think we're good.
|
|
||
| Sidekiq.configure_server do |config| | ||
| config.server_middleware do |chain| | ||
| chain.add(Datadog::Contrib::Sidekiq::Tracer, debug: true) |
There was a problem hiding this comment.
I assume that the add method appends the middleware at the bottom of the chain right? don't we have a way to keep it as the first one? in this way we can trace other middlewares execution time. If we can't, a suggestion in the docs is enough.
There was a problem hiding this comment.
Apparently the last added middlewares are executed first, and add() append at the end.
No description provided.