feature Integrate Shoryuken into ddtracer #626
Conversation
JustSnow
force-pushed
the
feature/integrate-shoryuken
branch
from
6a90844
to
4c8c790
Compare
There was a problem hiding this comment.
All-in-all this is a great start! I'm really pleased to see this contribution, and that you've already implemented the integration standard we had recently setup.
There are just some minor changes I'd suggest making (nothing serious.) Additionally we should add some RSpec tests and documentation to match the changes here before we merge.
We'll want to merge this to 0.18-dev, our feature branch, so you'll want to rebase it on that (which should also fix the broken tests you were seeing in the build.) If you add any other commits which kick off a build, don't worry if the "prerelease gem" steps fails; as long as all the other steps are green, we can call it ready.
Thanks for submitting this @JustSnow ! Looking forward to getting this in!
| @tracer = options[:tracer] || Datadog.configuration[:shoryuken][:tracer] | ||
| @shoryuken_service = options[:service_name] || Datadog.configuration[:shoryuken][:service_name] | ||
| @tracer.enabled = options[:enabled] || Datadog.configuration[:shoryuken][:enabled] | ||
| Datadog::Tracer.debug_logging = options[:debug] || Datadog.configuration[:shoryuken][:debug] |
There was a problem hiding this comment.
I don't think this option is necessary, users should be able to enable/disable tracer debug logging via the tracer itself.
| def initialize(options = {}) | ||
| @tracer = options[:tracer] || Datadog.configuration[:shoryuken][:tracer] | ||
| @shoryuken_service = options[:service_name] || Datadog.configuration[:shoryuken][:service_name] | ||
| @tracer.enabled = options[:enabled] || Datadog.configuration[:shoryuken][:enabled] |
There was a problem hiding this comment.
I don't think is option is necessary either.
| set_service_info(@shoryuken_service) | ||
|
|
||
| @tracer.trace(Ext::SPAN_JOB, service: @shoryuken_service, span_type: Datadog::Ext::AppTypes::WORKER) do |span| | ||
| span.resource = tracer_info[:resource] |
There was a problem hiding this comment.
These values should all be directly accessible within this block, right? If so, I'd suggest removing the tracer_info hash and access the values directly instead. This should save on a bit of memory allocation per call.
e.g.
span.set_tag(Ext::TAG_JOB_ID, sqs_msg.message_id)| tracer_info[:job_attribites] = sqs_msg.attributes | ||
| tracer_info[:body] = body | ||
|
|
||
| set_service_info(@shoryuken_service) |
There was a problem hiding this comment.
Since setting service info only needs to be done once, I might advise moving this to the constructor instead, which would prevent unnecessary checks and save a little performance.
| # Default settings for the Shoryuken integration | ||
| class Settings < Contrib::Configuration::Settings | ||
| option :service_name, default: Ext::SERVICE_NAME | ||
| option :debug, default: false |
There was a problem hiding this comment.
The debug and enabled options could also be removed here (as its related to the suggested change in tracer.rb)
JustSnow
force-pushed
the
feature/integrate-shoryuken
branch
from
4c8c790
to
e155dbc
Compare
JustSnow
force-pushed
the
feature/integrate-shoryuken
branch
from
e155dbc
to
fe8665b
Compare
|
@delner fixed comments. Will think about tests. If you have suggestions, would be good to see them :) |
|
@JustSnow For tests, I think we just need some basic RSpec tests that verify that it traces a job properly. You'd probably want to find a way to mock an incoming message from SQS (from within the test suite as opposed to setting up an external service), then use that to drive the test and verify it generated traces with the expected attributes. You might want to look at something like Racecar as an example. Also, could you try this out in an actual application and share a screenshot of how the trace looks in the UI? I'd very much like to see what it looks like in action! |
JustSnow
force-pushed
the
feature/integrate-shoryuken
branch
from
742a8b9
to
7044a5a
Compare
|
@JustSnow Overall this is looking very good; I think its all configured pretty much as we'd like to see it. We should add more tests that simulate a Shoryuken job running, which verify that it produces a span with the appropriate name, resource, service, and tags w/ values at minimum. There should be a number of other integrations you should be able to copy tests from that verify similar behavior, it'd just be a matter then of modifying them to have them trigger a Shoryuken job. Once we have that in, I think we can do a final review and merge this. |
|
@delner totally agree. working on it, but don't have enough time to finish tests |
JustSnow
force-pushed
the
feature/integrate-shoryuken
branch
2 times, most recently
from
37f7814
to
b4a1cf0
Compare
| end | ||
|
|
||
| before do | ||
| allow(Shoryuken::Client).to receive(:queues).with(queue).and_return sqs_queue |
There was a problem hiding this comment.
@delner maybe do you know or some one can suggest right way how to run shoryuken client for test to getting spans from writer
|
@JustSnow I created a diff.txt you can apply to this PR which should update it with a test that runs Shoryuken in a test. (Your repo didn't grant push permissions from the upstream, otherwise I would've pushed it myself.) Unfortunately it appears our tests for this won't work though for this because Shoryuken doesn't run middleware when running workers inline for tests. I opened an issue and we'll see if this gets merged/released for Shoryuken. We probably can merge and release this integration in the mean time, but we'll need to make sure we update the CI accordingly later. |
JustSnow
force-pushed
the
feature/integrate-shoryuken
branch
from
b4a1cf0
to
87eade8
Compare
|
@delner updated according |
No description provided.