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

Reset trace and transaction for sidekiq-cron #2446

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

frederikspang
Copy link
Contributor

@frederikspang frederikspang commented Oct 28, 2024

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

Copy link

codecov bot commented Oct 28, 2024

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
Copy link
Contributor Author

@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.

Copy link
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 :)

sentry-sidekiq/lib/sentry/sidekiq/cron/job.rb Outdated Show resolved Hide resolved
sentry-sidekiq/lib/sentry/sidekiq/cron/job.rb Show resolved Hide resolved
sentry-sidekiq/lib/sentry/sidekiq/cron/job.rb Show resolved Hide resolved
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
Copy link
Contributor Author

Some small things to improve/simplify :)

Replied to some, and adjusted for others! @solnic

@solnic
Copy link
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
Contributor Author

@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
Contributor Author

@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
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
140 of 142 checks passed
@frederikspang frederikspang deleted the feature/sidekiq-cron-reset branch November 5, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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