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

Shoryuken: use job name as resource instead of the wrapper #671

Merged
merged 2 commits into from
Jan 17, 2019

Conversation

aurelian
Copy link
Contributor

Currently all jobs are grouped within the same job wrapper, ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper

With this proposal, the resource changes from:

screenshot 2019-01-16 at 14 45 34

to

screenshot 2019-01-16 at 14 45 47

@delner
Copy link
Contributor

delner commented Jan 16, 2019

Ahh, yeah, this is an ActiveJob compatibility thing... we've seen this before, I think for Sidekiq. I'll take a deeper look at this a bit later, but it looks like this is a desirable change.

@delner delner self-requested a review January 16, 2019 15:26
@delner delner added integrations Involves tracing integrations community Was opened by a community member feature Involves a product feature labels Jan 16, 2019
@delner delner added this to the 0.18.3 milestone Jan 17, 2019
@delner
Copy link
Contributor

delner commented Jan 17, 2019

I think how it was written, body['job_class'] could raise an exception if the body was not a Hash or String. It not raising an error with String was complete coincidence too because it defines []; mostly a happy accident that this wasn't more problematic.

Modified the logic a bit for type checking and safety, and added some tests that actually assert whether this is a safe change: Shoryuken unit tests don't run properly in our test suite because it currently doesn't run middleware with the InlineExecutor. Hence we could be introducing bugs without knowing it. The tests I added should help guard against this, even if they're a bit brittle and not ideal.

Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

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

👍

@delner delner merged commit 545e5c1 into DataDog:master Jan 17, 2019
@aurelian
Copy link
Contributor Author

Thanks! Great job adding those tests!

@aurelian aurelian deleted the patch-1 branch January 18, 2019 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants