Skip to content

Reset trace and transaction for sidekiq-cron#2446

Merged
solnic merged 1 commit intogetsentry:masterfrom
frederikspang:feature/sidekiq-cron-reset
Nov 5, 2024
Merged

Reset trace and transaction for sidekiq-cron#2446
solnic merged 1 commit intogetsentry:masterfrom
frederikspang:feature/sidekiq-cron-reset

Conversation

@frederikspang
Copy link
Copy Markdown
Contributor

@frederikspang frederikspang commented Oct 28, 2024 •
edited
Loading

Description

Adds a new transaciton and reset for Sidekiq cron #enque!, so that they are not forever-running traces in the web view.

Depends on #2403.

Fixes #2391

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 28, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 98.13%. Comparing base (9446a30) to head (f9bce57).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
sentry-sidekiq/lib/sentry/sidekiq/cron/job.rb 90.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2446      +/-   ##
==========================================
- Coverage   98.16%   98.13%   -0.04%     
==========================================
  Files         126      126              
  Lines        4741     4761      +20     
==========================================
+ Hits         4654     4672      +18     
- Misses         87       89       +2
Components Coverage Δ
sentry-ruby 98.51% <ø> (ø)
sentry-rails 97.05% <ø> (ø)
sentry-sidekiq 96.96% <90.00%> (-0.67%) ⬇️
sentry-resque 92.85% <ø> (ø)
sentry-delayed_job 95.65% <ø> (ø)
sentry-opentelemetry 99.31% <ø> (ø)
Files with missing lines Coverage Δ
sentry-sidekiq/lib/sentry/sidekiq/cron/job.rb 94.59% <90.00%> (-5.41%) ⬇️

@frederikspang frederikspang force-pushed the feature/sidekiq-cron-reset branch from 8754868 to 1fe8563 Compare October 28, 2024 22:39
@frederikspang
Copy link
Copy Markdown
Contributor Author

frederikspang commented Oct 29, 2024

@solnic Any ideas or input for the define_method I've done?
I don't love it- But I also don't love the idea of identical method like

if Version >= 2
  def enqueue!
    ... 
  end
else
  def enque!
    ...
  end
end

with the exact same content of the two methods.

solnic
solnic requested changes Oct 29, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@solnic solnic left a comment

Choose a reason for hiding this comment

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

Some small things to improve/simplify :)

@frederikspang
Add childspan for Sidekiq Queue instrumentation
f9bce57 
Remove unneeded exception alloc

Set root op instead of nested spans

Initial commit

Add a spec

This is ugly - Want input

Inline allocation

Inline more

Add changelog entry

Feedback on PR
@frederikspang frederikspang force-pushed the feature/sidekiq-cron-reset branch from f8243d7 to f9bce57 Compare October 29, 2024 13:48
@frederikspang
Copy link
Copy Markdown
Contributor Author

frederikspang commented Oct 29, 2024

Some small things to improve/simplify :)

Replied to some, and adjusted for others! @solnic

@solnic
Copy link
Copy Markdown
Collaborator

solnic commented Oct 29, 2024

Hmm, it doesn't seem to be fixing the issue though :(
Screenshot 2024-10-29 at 21 04 46

@frederikspang
Copy link
Copy Markdown
Contributor Author

frederikspang commented Oct 29, 2024

@solnic That's weird...
I've tried this on our own sentry instance as well, that seems to be working.

Let me just validate tomorrow, and see what might go wrong.
Could be related to the define_method refactoring after latest PR feedback.

@frederikspang
Copy link
Copy Markdown
Contributor Author

frederikspang commented Nov 1, 2024

@solnic Are you on latest commit?

I've just tried this here, and my cron jobs look like this (Just from the traces view - But shows different trace ID's:

image

I'm not sure how to proceed debugging here.

@solnic
Copy link
Copy Markdown
Collaborator

solnic commented Nov 5, 2024

@solnic Are you on latest commit?

I've just tried this here, and my cron jobs look like this (Just from the traces view - But shows different trace ID's:

OK I tried again and I no longer see this issue! 🎉

@solnic solnic merged commit ceba0da into getsentry:master Nov 5, 2024
@frederikspang frederikspang deleted the feature/sidekiq-cron-reset branch November 5, 2024 18:35
@frederikspang frederikspang mentioned this pull request Nov 25, 2024
Closed
@pierrickouw
Copy link
Copy Markdown

pierrickouw commented Dec 16, 2024 •
edited
Loading

FWIW, we have seen this kind of error with sidekiq-cron < 2.0 (1.12), the latest version of sentry-sidekiq, and active_job

WARN: NoMethodError: undefined method `perform_async' for an instance of ActiveJob::ConfiguredJob

making our cron fails.

Updating sidekiq-cron to 2.0 fixes the issue.

@nateberkopec nateberkopec mentioned this pull request Jan 29, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@solnic solnic solnic approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Jobs created by Sidekiq-cron shouldn’t propagate trace header

3 participants

@frederikspang @solnic @pierrickouw