Shoryuken middleware tracer #577
Conversation
There was a problem hiding this comment.
Thanks so much for the contribution! Always nice to see these PRs, so I'd be happy to help you get this merged.
I think we have a good start here by utilizing the middleware, however, we'll have to make some changes to how the integration is activated and configured itself, while still maintaining the middleware pattern as you've added it here.
ddtrace has had its integrations going through some rework as of late. Our goal has been to create some standardization for consistency, so we can implement some integration features more broadly. To accomplish this, each of the integrations need to implement our new integration API, including Shoryuken.
This is a pretty big change from how older integrations used to work. However, the good news is I've been recently overhauling similar integrations for other job frameworks, so I have some sample code I can share with you that you could use as a template that you should be able to fit this middleware into.
You can see all of our integrations in this new format in #544 , so you should be able to copy from any of those that look most relevant. Sidekiq sounds like it might be the closest analog, so I might recommend starting there. Once we have this PR implementing that API, I'll take another look at this.
Side note: I wish I had some more documentation to share on how this works, but as it stands you've perfectly timed this PR with our new API, so unfortunately this is a bit of a guinea pig situation! 😬
In the mean time, if you have any questions about how this works, or if it sounds a bit overwhelming, I'm more than happy to assist, or undertake this ourselves by adding it to our backlog.
| def initialize(options = {}) | ||
| # check if Rails configuration is available and use it to override | ||
| # Shoryuken defaults | ||
| rails_config = ::Rails.configuration.datadog_trace rescue {} |
There was a problem hiding this comment.
What version of ddtrace was this written against? Rails.configuration.datadog_trace hasn't been used in a while, so I suspect we might want to rebase this PR off of the latest version. We are currently on 0.16.0 as of this comment.
There was a problem hiding this comment.
Also, other question: does Shoryuken depend on Rails? Or can it operate independently of a Rails application?
There was a problem hiding this comment.
Shoryuken is independent of Rails
There was a problem hiding this comment.
The implementation of this middleware was based on Contrib::Sidekiq (ddtrace-0.9.2), but there is no direct dependency of Rails. I'm going to change.
| else | ||
| tracer_info[:job_id] = worker_instance.job_id | ||
| end | ||
| set_service_info(@shoryuken_service) |
There was a problem hiding this comment.
Having this is good! It's a nuance that we require in our tracing solutions right now (although we eventually plan to remove this.)
This call only needs to be made once though. I do see the return in the function, but would this make more sense to move #initialize instead? A minor thing, but might avoid unnecessary calls or checks.
|
|
||
| @tracer.trace('shoryuken.job', service: @shoryuken_service, span_type: 'job') do |span| | ||
| span.resource = tracer_info[:resource] | ||
| span.set_tag('shoryuken.job.id', tracer_info[:job_id]) |
There was a problem hiding this comment.
Always good to see extra tags like this. 👍
|
|
||
| # configure Shoryuken service | ||
| tracer_info = { resource: worker_instance.class.name } | ||
| if worker_instance.class.name.ends_with?('ShoryukenAdapter::JobWrapper') |
There was a problem hiding this comment.
What's the nuance here between a Job and JobWrapper? How do these things relate? And how does it fit in with the rest of the framework?
|
|
||
| # set Tracer status | ||
| @tracer.enabled = user_config[:enabled] | ||
| Datadog::Tracer.debug_logging = user_config[:debug] |
There was a problem hiding this comment.
Why expose and set the debug logging flag here? Is a user not able to otherwise set this flag directly on the tracer in their configuration?
delner
added
integrations
|
We merged #626 which implements Shoryuken tracing in |
Feature support for #538.