Skip to content

Add Sentry.with_child_span for easier span recording #1783

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

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Apr 4, 2022

This PR adds a new Sentry.with_child_span API. It'll allow users to record a child span with a block without the need to getting the current span from scope, checking its presence...etc. A simple block wrapping will do the trick:

Sentry.with_child_span(op: "my op") do |child_span|
  # my operation
  # operation result will be the return value
end

Note that if there's no span in the current scope, the block will still be executed. But the yield child_span will be nil instead.

With this API, it's also easier to fix the child span nesting issue reported in #1723.

@st0012 st0012 added this to the 5.3.0 milestone Apr 4, 2022
@st0012 st0012 self-assigned this Apr 4, 2022
@st0012 st0012 requested a review from sl0thentr0py April 4, 2022 20:37
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022

Codecov Report

Merging #1783 (4af72c0) into master (fa46e86) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1783      +/-   ##
==========================================
- Coverage   98.40%   98.40%   -0.01%     
==========================================
  Files         145      145              
  Lines        8480     8536      +56     
==========================================
+ Hits         8345     8400      +55     
- Misses        135      136       +1     
Impacted Files Coverage Δ
sentry-ruby/lib/sentry-ruby.rb 96.22% <100.00%> (+0.25%) ⬆️
...y-ruby/spec/sentry/rack/capture_exceptions_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry_spec.rb 99.76% <100.00%> (+0.01%) ⬆️
sentry-resque/lib/sentry/resque.rb 97.14% <0.00%> (-2.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa46e86...4af72c0. Read the comment docs.

@st0012
Copy link
Collaborator Author

st0012 commented Apr 7, 2022

ping @sl0thentr0py


begin
current_span.with_child_span(**attributes) do |child_span|
get_current_scope.set_span(child_span)
Copy link
Member

Choose a reason for hiding this comment

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

Just an observation, all the current usages of set_span set it to the transaction and we don't really use scope#get_span either so this will be the first time the span on the scope gets used right?

➜  sentry-ruby git:(master) ✗ ag --ignore 'spec' set_span
sentry-rails/lib/sentry/rails/action_cable.rb
18:              scope.set_span(transaction) if transaction

sentry-rails/lib/sentry/rails/active_job.rb
31:                scope.set_span(transaction) if transaction

sentry-resque/lib/sentry/resque.rb
28:              scope.set_span(transaction) if transaction

sentry-delayed_job/lib/sentry/delayed_job/plugin.rb
22:            scope.set_span(transaction) if transaction

sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb
21:        scope.set_span(transaction) if transaction

sentry-ruby/lib/sentry/scope.rb
132:    def set_span(span)

sentry-ruby/lib/sentry/rack/capture_exceptions.rb
23:            scope.set_span(transaction) if transaction

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, because most the usages in the SDK are to create the top-level transaction. But as described in #1784, sentry-rails's subscriber should use spans instead. And users will generally use get_span and set_span for spans more than transactions.

Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

sorry for the delay, busy week :/

can you maybe add an integration test too where you add a child span with this new api in an existing request/response cycle transaction?

@st0012 st0012 force-pushed the add-sentry-with-child-span branch from 0807404 to 4af72c0 Compare April 8, 2022 08:28

second_span = transaction.spans.last
expect(second_span[:op]).to eq("second level")
expect(second_span[:parent_span_id]).to eq(first_span[:span_id])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sl0thentr0py I've added the integration test here.

@st0012 st0012 requested a review from sl0thentr0py April 8, 2022 08:33
Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

thx!

@st0012 st0012 merged commit a1ea253 into master Apr 8, 2022
@st0012 st0012 deleted the add-sentry-with-child-span branch April 8, 2022 08:36
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.

3 participants