Nested childs for Active Record SQL queries

[pbernery]

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?

[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]

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

[st0012]

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]

@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]

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]

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]

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]

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]

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

[st0012]

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

[st0012]

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]

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

[st0012]

@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]

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]

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