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

Add support for distributed tracing for DelayedJob #2233

Merged
merged 2 commits into from
Jan 28, 2024

Conversation

sl0thentr0py
Copy link
Member

@sl0thentr0py sl0thentr0py commented Jan 16, 2024

  • works for PerformableMethod
  • activejob will be done more generally

part of #1904

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Merging #2233 (8485799) into master (a9fcc38) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2233      +/-   ##
==========================================
+ Coverage   97.39%   97.43%   +0.03%     
==========================================
  Files         102      102              
  Lines        3804     3823      +19     
==========================================
+ Hits         3705     3725      +20     
+ Misses         99       98       -1     
Components Coverage Δ
sentry-ruby 98.13% <ø> (+0.03%) ⬆️
sentry-rails 95.05% <ø> (ø)
sentry-sidekiq 94.70% <ø> (ø)
sentry-resque 92.06% <ø> (ø)
sentry-delayed_job 95.60% <100.00%> (+1.15%) ⬆️
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
...entry-delayed_job/lib/sentry/delayed_job/plugin.rb 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@sl0thentr0py sl0thentr0py force-pushed the neel/delayed-job-tracing branch 5 times, most recently from 354e8fa to 534730d Compare January 17, 2024 14:02
@sl0thentr0py sl0thentr0py changed the title WIP delayed job tracing Add support for distributed tracing for DelayedJob Jan 17, 2024
@sl0thentr0py sl0thentr0py marked this pull request as ready for review January 17, 2024 14:03
Copy link
Collaborator

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Instead of injecting trace data to args, can we maybe use a custom ivar? Like @_sentry_trace_data. That way the extraction will be easier and we don't need to risk breaking the call completely if the args deletion somehow goes wrong.

sentry-delayed_job/lib/sentry/delayed_job/plugin.rb Outdated Show resolved Hide resolved
@sl0thentr0py
Copy link
Member Author

sl0thentr0py commented Jan 22, 2024

Instead of injecting trace data to args, can we maybe use a custom ivar?

we'll need to patch delayed_job's YAML stuff here then, since ivar's are not picked up by default and won't survive the database round trip.
https://github.com/collectiveidea/delayed_job/blob/master/lib/delayed/psych_ext.rb#L4-L10

so I opted for this, which of the two do you prefer?

@st0012
Copy link
Collaborator

st0012 commented Jan 22, 2024

we'll need to patch delayed_job's YAML stuff here then

Ah thanks for this context. Let's first go with the current implementation then.

sl0thentr0py and others added 2 commits January 23, 2024 15:11
* only for `PerformableMethod` here, active job will be handled
  generally separately
@@ -1,4 +1,5 @@
require "bundler/setup"
require "debug" if RUBY_VERSION.to_f >= 2.6 && RUBY_ENGINE == "ruby"
require "pry"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related: do you still use pry?

Copy link
Member Author

@sl0thentr0py sl0thentr0py Jan 28, 2024

Choose a reason for hiding this comment

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

nope! I've moved to debugger if available

@sl0thentr0py sl0thentr0py merged commit 3135516 into master Jan 28, 2024
122 checks passed
@sl0thentr0py sl0thentr0py deleted the neel/delayed-job-tracing branch January 28, 2024 23:17
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.

2 participants