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

Sidekiq instrumentation: evaluate covering workflows other than perform_async #2195

Closed
fallwith opened this issue Sep 8, 2023 · 5 comments
Closed
Assignees
Labels
3 Story Point Estimate enhancement Enhancement to existing features

Comments

@fallwith
Copy link
Contributor

fallwith commented Sep 8, 2023

Our APM Sidekiq instrumentation has coverage for Sidekiq clients and Sidekiq servers.

For the Sidekiq client instrumentation piece, we really only set a DT header in the job metadata, so the Sidekiq server instrumentation piece needs to be engaged before we can glean proper job metrics.

Because of this setup, any job performed synchronously runs the risk of not being instrumented properly given that the server instrumentation is not engaged.

For example:

# engages client + server, works great
DeadEndJob.perform_async(args)

# engages client, instrumentation lacking
DeadEndJob.perform_inline(args)

# engages neither, instrumentation lacking
DeadEndJob.new.perform(args)

We should evaluate the level of effort required to cover the use cases that are currently unsupported.

@fallwith fallwith added the feature request To tag feature request after Hero Triage for PM to disposition label Sep 8, 2023
@workato-integration
Copy link

workato-integration bot commented Sep 8, 2023

@fallwith fallwith added estimate Issue needing estimation oct-dec qtr Possible FY Q3 candidate enhancement Enhancement to existing features and removed feature request To tag feature request after Hero Triage for PM to disposition labels Sep 9, 2023
@kford-newrelic kford-newrelic added 3 Story Point Estimate and removed estimate Issue needing estimation labels Sep 14, 2023
@kford-newrelic kford-newrelic added jan-mar qtr Possible FY Q4 candidate and removed oct-dec qtr Possible FY Q3 candidate labels Oct 2, 2023
@kford-newrelic kford-newrelic removed the jan-mar qtr Possible FY Q4 candidate label Jan 10, 2024
@kaylareopelle
Copy link
Contributor

Re: #2379, add perform_now to the list of methods to investigate

@fallwith
Copy link
Contributor Author

Using perform_now works just fine. I'm not sure what was causing it not to earlier, as the v6.4.2 version of Sidekiq that was around back then offers a perform_now method that engages with all registered client middleware and server middleware, just like the current version of the method does. It is the engagement of our server middleware that produces a segment. I will add a unit test to demonstrate perform_now working well.

Using MyJobClass.new.perform can't possibly work because that perform method involves 100% Sidekiq user authored code and does not by default engage any Sidekiq middleware. Apart from some minimum requirements about the arguments that the perform method needs to accept, the body of the method can truly do anything the Sidekiq user wishes it to. I currently think that invoking #perform on a job class instance directly is for testing or one-off debugging scenarios and is it a bit of a production context anti-pattern. Users who wish to directly fire off a job without queuing it up first should leverage perform_inline which the agent instruments just fine.

As for perform_now and perform_later, those are ActiveJob-isms. Our Sidekiq suite currently runs outside of Rails and our Rails tests currently don't leverage Sidekiq, so we'd need to set something up that mixes the two.

@fallwith
Copy link
Contributor Author

Pushing the ActiveJob related testing of perform_now and perform_later off to #2835.

@fallwith
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 Story Point Estimate enhancement Enhancement to existing features
Projects
Archived in project
Development

No branches or pull requests

3 participants