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 child_spans for Sidekiq Queue instrumentation #2403

Merged

Conversation

frederikspang
Copy link
Contributor

@frederikspang frederikspang commented Sep 16, 2024

Implements #2322 for Sidekiq.

Description

Adds support for Sentry Queues page (With predefined span op names)

https://docs.sentry.io/platforms/ruby/guides/sidekiq/tracing/instrumentation/custom-instrumentation/queues-module/

Screenshot 2024-09-16 at 15 45 15

@frederikspang frederikspang force-pushed the feature/sidekiq-queues-childspan branch 2 times, most recently from cd35fcf to 787436b Compare September 16, 2024 16:02
@frederikspang frederikspang force-pushed the feature/sidekiq-queues-childspan branch 3 times, most recently from ac46c1b to 4e505f6 Compare September 17, 2024 09:07
@rwojsznis
Copy link

Oh wow, exactly what I was looking for 😅 🙌

can we somehow help to get this shipped? 💪

Copy link
Collaborator

@solnic solnic left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! 🙇🏻 Would you be able to add some basic specs for this too?

@frederikspang
Copy link
Contributor Author

@solnic I've added two basic specs.
Latency is a bit difficult, without introducing some timecop kind of tool. Which we do have in sentry-ruby, so we could just introduce it here if we wanted to.

@frederikspang
Copy link
Contributor Author

@solnic After looking at these, I think queue.process may actually replace op: queue.sidekiq instead of being a child span.
They will always be redundant - But we should set origin or some other field to sidekiq, description or name maybe, to differentiate the processor. (ie. using another processor alongside Sidekiq - or doing queries in Sentry UI across applications)

image

@nateberkopec
Copy link
Contributor

Looks like it needs a changelog too

@frederikspang frederikspang force-pushed the feature/sidekiq-queues-childspan branch from 04750fa to af385ba Compare October 21, 2024 20:51
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.16%. Comparing base (27d7384) to head (5e000c6).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2403   +/-   ##
=======================================
  Coverage   98.15%   98.16%           
=======================================
  Files         126      126           
  Lines        4728     4741   +13     
=======================================
+ Hits         4641     4654   +13     
  Misses         87       87           
Components Coverage Δ
sentry-ruby 98.51% <ø> (ø)
sentry-rails 97.05% <ø> (ø)
sentry-sidekiq 97.63% <100.00%> (+0.15%) ⬆️
sentry-resque 92.85% <ø> (ø)
sentry-delayed_job 95.65% <ø> (ø)
sentry-opentelemetry 99.31% <ø> (ø)
Files with missing lines Coverage Δ
...iq/lib/sentry/sidekiq/sentry_context_middleware.rb 98.21% <100.00%> (+0.53%) ⬆️

@solnic
Copy link
Collaborator

solnic commented Oct 25, 2024

@solnic After looking at these, I think queue.process may actually replace op: queue.sidekiq instead of being a child span.
They will always be redundant

  • But we should set origin or some other field to sidekiq, description or name maybe, to differentiate the processor. (ie. using another processor alongside Sidekiq - or doing queries in Sentry UI across applications)

I've been testing this out and I don't see queue.sidekiq/queue.process. This is what I see instead:

Screenshot 2024-10-25 at 14 51 35

@solnic
Copy link
Collaborator

solnic commented Oct 25, 2024

Latency is a bit difficult, without introducing some timecop kind of tool. Which we do have in sentry-ruby, so we could just introduce it here if we wanted to.

Thanks for adding the specs. I think it would be good to use timecop there actually.

@frederikspang
Copy link
Contributor Author

@solnic After looking at these, I think queue.process may actually replace op: queue.sidekiq instead of being a child span.
They will always be redundant

  • But we should set origin or some other field to sidekiq, description or name maybe, to differentiate the processor. (ie. using another processor alongside Sidekiq - or doing queries in Sentry UI across applications)

I've been testing this out and I don't see queue.sidekiq/queue.process. This is what I see instead:

Screenshot 2024-10-25 at 14 51 35

You're right! I changed that after my comment, before your review here:

cd3a1df

I'll have a look at the other comments as soon as possible.

@frederikspang
Copy link
Contributor Author

frederikspang commented Oct 27, 2024

@solnic I have updated PR with feedback adjusted!

Edit: I see specs are broken. I'll look into that as well!

@frederikspang frederikspang force-pushed the feature/sidekiq-queues-childspan branch 2 times, most recently from 9aaf80b to 671dd65 Compare October 28, 2024 09:52
@frederikspang
Copy link
Contributor Author

@solnic The specs failing seems related to sidekiq/sidekiq#6430 - Was able to reproduce locally, after clearing Lock file and bundle install again.

Somewhat unrelated to this PR, although it's noisy.

Copy link
Collaborator

@solnic solnic left a comment

Choose a reason for hiding this comment

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

Great job thank you!

@solnic
Copy link
Collaborator

solnic commented Oct 28, 2024

@solnic The specs failing seems related to sidekiq/sidekiq#6430 - Was able to reproduce locally, after clearing Lock file and bundle install again.

Somewhat unrelated to this PR, although it's noisy.

Please update to the latest master. I just checked locally and it passes. Once CI is green I'll merge it in.

@frederikspang frederikspang force-pushed the feature/sidekiq-queues-childspan branch from d3a7219 to 265307a Compare October 28, 2024 22:29
@frederikspang
Copy link
Contributor Author

@solnic Done!

@solnic solnic merged commit 9446a30 into getsentry:master Oct 29, 2024
140 of 141 checks passed
@frederikspang frederikspang deleted the feature/sidekiq-queues-childspan branch October 29, 2024 13:35
@nateberkopec
Copy link
Contributor

🙌 Thanks everyone! This is such a great feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants