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 delayed job context into the sampling context #2148

Merged

Conversation

rlineweaver
Copy link
Contributor

@rlineweaver rlineweaver commented Oct 19, 2023

Description

This makes additional job context available in the sampling_context via the Sentry transaction's custom_sampling_context, which makes it available for making sampling rate determinations in traces_sampler. In our case, we want to base the sampling rate off of the job's priority. This PR allows you to write code like:

    config.traces_sampler = lambda do |sampling_context|
        sample_rate = 1

        if sampling_context[:transaction_context][:op] == Sentry::DelayedJob::Plugin::OP_NAME
           job_priority = sampling_context.dig(Sentry::DelayedJob::Plugin::DELAYED_JOB_CONTEXT_KEY, :priority)
           if job_priority && job_priority >= 99
              sample_rate = 0.01
           end
        end

        sample_rate
   end

@rlineweaver rlineweaver changed the title copy delayed job context into the transaction via custom_sampling_context add delayed job context into the transaction via custom_sampling_context Oct 19, 2023
@rlineweaver rlineweaver changed the title add delayed job context into the transaction via custom_sampling_context add delayed job context into the sampling context Oct 19, 2023
@booleanbetrayal
Copy link

Agree that this could be super useful!

Copy link

@booleanbetrayal booleanbetrayal left a comment

Choose a reason for hiding this comment

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

Code changes LGTM and everything has been tested!

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #2148 (9da2d74) into master (67412c4) will increase coverage by 0.02%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2148      +/-   ##
==========================================
+ Coverage   97.27%   97.30%   +0.02%     
==========================================
  Files          97       97              
  Lines        3632     3634       +2     
==========================================
+ Hits         3533     3536       +3     
+ Misses         99       98       -1     
Components Coverage Δ
sentry-ruby 97.98% <ø> (ø)
sentry-rails 94.98% <100.00%> (+0.01%) ⬆️
sentry-sidekiq 93.70% <ø> (ø)
sentry-resque 93.65% <ø> (+1.58%) ⬆️
sentry-delayed_job 94.36% <100.00%> (ø)
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
...entry-delayed_job/lib/sentry/delayed_job/plugin.rb 100.00% <100.00%> (ø)
...b/sentry/rails/breadcrumb/active_support_logger.rb 100.00% <100.00%> (ø)
sentry-rails/lib/sentry/rails/configuration.rb 97.14% <100.00%> (+0.26%) ⬆️
sentry-rails/lib/sentry/rails/railtie.rb 98.68% <100.00%> (ø)

... and 1 file with indirect coverage changes

@st0012
Copy link
Collaborator

st0012 commented Oct 23, 2023

@sl0thentr0py In other SDKs, is there a format to add job contexts as sampling context? Perhaps we should adopt this in other integrations as well? If so, then we should probably add delayed_job as the key here to distinguish context from different jobs given that some apps do use multiple background job libraries.

@sl0thentr0py
Copy link
Member

@rlineweaver
Copy link
Contributor Author

@st0012 yes let's put it under delayed_job

Hi @st0012 , @sl0thentr0py , thanks for looking at this.

There are at most two keys being set in the custom_sampling_context here: :"Delayed-Job" and :"Active-Job" (the value of each is another hash containing details about the job). So I think these keys are already namespaced well enough to avoid confusion/collisions. I'd be happy to rename those keys to something else (like delayed_job), but I reused the existing keys for consistency with the scope's context.

@sl0thentr0py
Copy link
Member

ok fine with me then!

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.

Ah I see. I now regret defining those keys with camelcase + dash. But I guess it's too late to change 🤦‍♂️

@st0012
Copy link
Collaborator

st0012 commented Oct 28, 2023

@rlineweaver Thank you for this PR 🙌

@st0012 st0012 merged commit dbbc7e4 into getsentry:master Oct 28, 2023
97 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants