Skip to content

add delayed job context into the sampling context#2148

Merged
st0012 merged 4 commits intogetsentry:masterfrom
rlineweaver:copy-delayed-job-context-into-transaction
Oct 28, 2023
Merged

add delayed job context into the sampling context#2148
st0012 merged 4 commits intogetsentry:masterfrom
rlineweaver:copy-delayed-job-context-into-transaction

Conversation

@rlineweaver
Copy link
Copy Markdown
Contributor

@rlineweaver rlineweaver commented Oct 19, 2023 •
edited
Loading

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
Copy Markdown

booleanbetrayal commented Oct 23, 2023

Agree that this could be super useful!

booleanbetrayal
booleanbetrayal approved these changes Oct 23, 2023
View reviewed changes
Copy link
Copy Markdown

@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
Copy Markdown

codecov bot commented Oct 23, 2023 •
edited
Loading

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
Copy Markdown
Contributor

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
Copy Markdown
Member

sl0thentr0py commented Oct 24, 2023

@st0012 yes let's put it under delayed_job, this is celery in python for instance
https://github.com/getsentry/sentry-python/blob/f570a9966252920bdb221101d596eb029497b0e9/sentry_sdk/integrations/celery.py#L247-L254

@rlineweaver
Copy link
Copy Markdown
Contributor Author

rlineweaver commented Oct 24, 2023

@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
Copy Markdown
Member

sl0thentr0py commented Oct 24, 2023

ok fine with me then!

st0012
st0012 approved these changes Oct 28, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

st0012 commented Oct 28, 2023

@rlineweaver Thank you for this PR 🙌

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

Reviewers

2 more reviewers

@booleanbetrayal booleanbetrayal booleanbetrayal approved these changes

@st0012 st0012 st0012 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rlineweaver @booleanbetrayal @st0012 @sl0thentr0py