Nested childs for Active Record SQL queries by pbernery · Pull Request #1723 · getsentry/sentry-ruby

pbernery · 2022-02-11T16:55:10Z

Currently, when nesting children (using start_child), SQL queries, Redis commands, are not attached to the created span but are attached to the root span (or the transaction).

This makes the span graph looks like this:

while I expect to have "db.sql.active_record" spans nested in parent spans.

I made some change in this PR that leads to the expected behaviour:

but I am not sure than calling set_span is the right approach, it changes the "current" span globally. This works when there is one nested branch but if there are multiple, this may not work as expected.

The Redis spans have the same issue:

And I guess the issue is everywhere Sentry.get_current_scope.get_transaction is used.

There was a PR #1382 that fixed nested custom child. It fixed things for children created manually, and proposes to use a child to create another child, but how could events handled by Sentry use those child?

…ted childs

st0012

I generally agree with this idea. But I'm not sure if it could cause problems to some users?

In my other project, there could be many n+1 queries across different abstraction layers in problematic endpoints. In those cases, I'm worry that nesting will make the spans harder to view? (Of course, you can also argue that it's an UI design issue).

@sl0thentr0py Do you think the above would be an issue? Is there any recommendation on nesting spans in integrations?

st0012 · 2022-02-16T21:28:16Z

sentry-rails/lib/sentry/rails/tracing/abstract_subscriber.rb

+            return unless current_span && current_span.sampled
+
+            span = current_span.start_child(**options)
+            Sentry.get_current_scope.set_span(span)


I think this and the get_current_scope are unnecessary. Aren't they the same scope as the scope local?

st0012 · 2022-02-16T21:28:47Z

sentry-rails/lib/sentry/rails/tracing/abstract_subscriber.rb

+            current_span = scope.get_span
+            return unless current_span && current_span.sampled
+
+            span = current_span.start_child(**options)


We can use with_child_span(&block) here:

sentry-ruby/sentry-ruby/lib/sentry/span.rb

Lines 151 to 166 in 341b66e

# Starts a child span, yield it to the given block, and then finish the span after the block is executed.

# @example

# span.with_child_span do |child_span|

# # things happen here will be recorded in a child span

# end

#

# @param attributes [Hash] the attributes for the child span.

# @param block [Proc] the action to be recorded in the child span.

# @yieldparam child_span [Span]

def with_child_span(**attributes, &block)

child_span = start_child(**attributes)

yield(child_span)

child_span.finish

end

sl0thentr0py · 2022-02-17T15:46:56Z

@st0012 I think the correct semantic is nesting, irrespective of UI concerns. If the DB query runs within some other span's context, it should certainly not go under the root span but be nested at the appropriate level.

pbernery · 2022-02-18T13:20:54Z

Thanks for both your feedbacks.

So, with @st0012 suggestions applied, do you believe this PR could be merged as-is then, or should more work be done?

The changes have made works for SQL transactions, but not for Redis transactions, so I guess the equivalent should be applied on the Redis side. And maybe more services.

But we could advance step by step: nest SQL first, then later Redis, etc.

codecov-commenter · 2022-02-20T15:11:41Z

Codecov Report

Merging #1723 (b30d84a) into master (c65088f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1723      +/-   ##
==========================================
- Coverage   98.41%   98.39%   -0.02%     
==========================================
  Files         141      141              
  Lines        8004     8032      +28     
==========================================
+ Hits         7877     7903      +26     
- Misses        127      129       +2

Impacted Files	Coverage Δ
...ls/lib/sentry/rails/tracing/abstract_subscriber.rb	`78.12% <100.00%> (+1.45%)`	⬆️
sentry-ruby/lib/sentry/background_worker.rb	`94.59% <0.00%> (-5.41%)`	⬇️
sentry-ruby/lib/sentry/hub.rb	`100.00% <0.00%> (ø)`
sentry-resque/lib/sentry/resque.rb	`100.00% <0.00%> (ø)`
sentry-rails/lib/sentry/rails/active_job.rb	`100.00% <0.00%> (ø)`
sentry-ruby/spec/initialization_check_spec.rb	`100.00% <0.00%> (ø)`
...y-ruby/spec/sentry/rack/capture_exceptions_spec.rb	`100.00% <0.00%> (ø)`
sentry-rails/spec/support/test_rails_app/app.rb
...ntry-rails/spec/support/test_rails_app/apps/6-1.rb
sentry-rails/spec/dummy/test_rails_app/app.rb	`92.06% <0.00%> (ø)`
... and 5 more

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 c65088f...b30d84a. Read the comment docs.

github-actions · 2022-03-14T00:11:20Z

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

pbernery · 2022-03-14T12:54:28Z

So what do you think @sl0thentr0py @st0012?
Should I propose this nested child change (by updating the code with your comments first) or should we close this proposition?

st0012 · 2022-03-20T11:12:29Z

@pbernery the proposal is good but we can't merge the current code and without tests.

st0012 · 2022-03-20T12:22:46Z

Let me know if you plan to finish the PR, or I can make another one to address the issue as well.

st0012 · 2022-04-03T12:31:33Z

I think the SDK should actually provide a top-level helper like Sentry.with_child_span { |span| ... } to do all the checks and heavy lifting work. So I will open another PR and make the change.

pbernery · 2022-04-04T08:39:17Z

Hi Stan, sorry for the delayed response.
Alright, sounds good, and looking forward to this update.

st0012 · 2022-04-08T08:43:41Z

@pbernery I've added #1784 to address the issue. Can you give it a try when have time? thx
I also introduced the Sentry.with_child_span API in #1783, which I think you'd want to take a look to 🙂

pbernery · 2022-04-22T15:02:48Z

Hey @st0012, just tested the Sentry.with_child_span and it looks like it's working great on our side.
sentry-redis may need the same change to nest calls the same way.

Thank you very much for this update :)

st0012 · 2022-04-23T11:04:59Z

@pbernery Thanks for catching that 👍 I've added another PR to address it.

Simple update demonstrating how the code may be written to handle nes…

b30d84a

…ted childs

st0012 reviewed Feb 16, 2022

View reviewed changes

github-actions bot added the Status: Stale label Mar 14, 2022

st0012 closed this Apr 3, 2022

This was referenced Apr 4, 2022

Add Sentry.with_child_span for easier span recording #1783

Merged

Fix sentry-rails's tracing spans not nesting issue #1784

Merged

st0012 added a commit that referenced this pull request Apr 9, 2022

Merge branch 'master' into fix-#1723

42308ad

st0012 mentioned this pull request Apr 23, 2022

Fix net-http tracing's span nesting issue #1796

Merged

	# Starts a child span, yield it to the given block, and then finish the span after the block is executed.
	# @example
	# span.with_child_span do \|child_span\|
	# # things happen here will be recorded in a child span
	# end
	#
	# @param attributes [Hash] the attributes for the child span.
	# @param block [Proc] the action to be recorded in the child span.
	# @yieldparam child_span [Span]
	def with_child_span(**attributes, &block)
	child_span = start_child(**attributes)

	yield(child_span)

	child_span.finish
	end

Uh oh!

Conversation

pbernery commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

st0012 left a comment

Choose a reason for hiding this comment

Uh oh!

st0012 Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

st0012 Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

sl0thentr0py commented Feb 17, 2022

Uh oh!

pbernery commented Feb 18, 2022

Uh oh!

codecov-commenter commented Feb 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Mar 14, 2022

Uh oh!

pbernery commented Mar 14, 2022

Uh oh!

st0012 commented Mar 20, 2022

Uh oh!

st0012 commented Mar 20, 2022

Uh oh!

st0012 commented Apr 3, 2022

Uh oh!

pbernery commented Apr 4, 2022

Uh oh!

st0012 commented Apr 8, 2022

Uh oh!

pbernery commented Apr 22, 2022

Uh oh!

st0012 commented Apr 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pbernery commented Feb 11, 2022 •

edited

Loading

codecov-commenter commented Feb 20, 2022 •

edited

Loading