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

Load the Sidekiq client tracer on Sidekiq server #1084

Closed
wants to merge 1 commit into from
Closed

Load the Sidekiq client tracer on Sidekiq server #1084

wants to merge 1 commit into from

Conversation

MXfive
Copy link

@MXfive MXfive commented Jun 17, 2020

Instrument the Sidekiq client when running a Sidekiq server, Sidekiq jobs can schedule other jobs.

See documentation here: https://github.com/mperham/sidekiq/wiki/Middleware#client-middleware-registered-in-both-places

@MXfive MXfive requested a review from a team June 17, 2020 09:26
phureewat29
phureewat29 previously approved these changes Jun 17, 2020
@delner
Copy link
Contributor

delner commented Jun 18, 2020

@MXfive Looks good to me! We need to get CI to pass before we merge this. Looks like an unrelated issue with Excon might be causing the failures. Once we have that fixed, you'll need to rebase.

@delner delner self-requested a review June 18, 2020 15:46
@delner delner added community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations labels Jun 18, 2020
@marcotc
Copy link
Member

marcotc commented Jun 18, 2020

Hey @MXfive, you can now rebase with master and the excon CI error should be gone.

Also, could you add a test to assert that these changes are working correctly?
I suggest creating a worker class that schedules another job, similar to how we test our ErrorWorker, for example:

class ErrorWorker
include Sidekiq::Worker
def perform
raise TestError, 'job error'
end
end

Instrument the Sidekiq client when running a Sidekiq server, Sidekiq jobs can schedule other jobs.
@MXfive
Copy link
Author

MXfive commented Jun 19, 2020

@marcotc Sure I will have a go at adding test coverage this weekend. Still looks like jruby tests is broken though for changes unrelated to this.

@marcotc
Copy link
Member

marcotc commented Jul 8, 2020

👋 @MXfive, I've recently merged #1092, which migrated our Sidekiq tests from Minitest to RSpec, so testing should be much easier now.

@DocX
Copy link
Contributor

DocX commented Jul 15, 2020

Is this not a duplicate of #1099 ?

@marcotc
Copy link
Member

marcotc commented Jul 15, 2020

Well noted @DocX, I'm going to close this issue as it had been addressed by #1099. Thank you for you work here @MXfive, the changes will be shipped on next release.

@marcotc marcotc closed this Jul 15, 2020
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.

5 participants