Skip to content

Experimental support for multi-threaded profiling using Vernier #2372

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Oct 4, 2024
Merged

Experimental support for multi-threaded profiling using Vernier #2372

merged 21 commits into from
Oct 4, 2024

Conversation

solnic
Copy link
Collaborator

@solnic solnic commented Aug 14, 2024 •
edited
Loading

Experimental support for Vernier.

Screenshots

Screenshot 2024-08-21 at 13 41 53 Screenshot 2024-08-21 at 13 37 24 Screenshot 2024-08-21 at 13 38 07 Screenshot 2024-08-21 at 13 39 04 Screenshot 2024-08-21 at 13 39 34

@solnic solnic linked an issue Aug 14, 2024 that may be closed by this pull request
Try vernier for profiling #2124
Closed
@codecov Codecov
Copy link

codecov bot commented Aug 14, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 99.25558% with 3 lines in your changes missing coverage. Please review.

Project coverage is 98.66%. Comparing base (a070e08) to head (7c8bfb1).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
sentry-ruby/spec/sentry/configuration_spec.rb 71.42% 2 Missing ⚠️
sentry-ruby/lib/sentry/configuration.rb 90.90% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2372      +/-   ##
==========================================
- Coverage   98.68%   98.66%   -0.03%     
==========================================
  Files         211      218       +7     
  Lines       13987    14337     +350     
==========================================
+ Hits        13803    14145     +342     
- Misses        184      192       +8
Components Coverage Δ
sentry-ruby 99.01% <99.25%> (-0.04%) ⬇️
sentry-rails 97.34% <ø> (ø)
sentry-sidekiq 97.07% <ø> (ø)
sentry-resque 96.79% <ø> (-0.33%) ⬇️
sentry-delayed_job 98.92% <ø> (ø)
sentry-opentelemetry 100.00% <ø> (ø)
Files with missing lines Coverage Δ
sentry-ruby/lib/sentry-ruby.rb 96.72% <100.00%> (+0.01%) ⬆️
sentry-ruby/lib/sentry/profiler.rb 100.00% <100.00%> (+0.88%) ⬆️
sentry-ruby/lib/sentry/profiler/helpers.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/transaction.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/transaction_event.rb 100.00% <ø> (ø)
sentry-ruby/lib/sentry/vernier/output.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/vernier/profiler.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/profiler_spec.rb 100.00% <100.00%> (ø)
...y-ruby/spec/sentry/rack/capture_exceptions_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/vernier/profiler_spec.rb 100.00% <100.00%> (ø)
... and 3 more

... and 4 files with indirect coverage changes

@solnic solnic force-pushed the solnic/2124-try-vernier-for-profiling branch 5 times, most recently from 3d995d8 to b45c7f3 Compare August 22, 2024 07:25
@solnic solnic changed the title Beginning of Vernier profiler Experimental support for multi-threaded profiling using Vernier Aug 22, 2024
@solnic solnic force-pushed the solnic/2124-try-vernier-for-profiling branch 5 times, most recently from 5220e97 to bd687cc Compare August 26, 2024 09:39
@solnic solnic marked this pull request as ready for review August 26, 2024 10:13
@solnic solnic requested a review from sl0thentr0py August 26, 2024 10:34
sl0thentr0py
sl0thentr0py requested changes Aug 27, 2024
View reviewed changes
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.

amazing work @solnic
did a first pass, some questions and comments

will do a detailed test round later in the week

st0012
st0012 reviewed Aug 30, 2024
View reviewed changes
@solnic solnic force-pushed the solnic/2124-try-vernier-for-profiling branch from 579dfef to babc6ed Compare August 31, 2024 08:37
@solnic
Copy link
Collaborator Author

solnic commented Aug 31, 2024

@sl0thentr0py @st0012 thanks for the comments! There are a couple of things that need be fixed. I'll ping you once it's done next week.

@solnic solnic force-pushed the solnic/2124-try-vernier-for-profiling branch from 6cc823c to 3a9367b Compare September 2, 2024 07:16
@solnic solnic force-pushed the solnic/2124-try-vernier-for-profiling branch from 4d2785b to dd7fdf9 Compare September 2, 2024 09:40
st0012
st0012 approved these changes Sep 2, 2024
View reviewed changes
Copy link
Collaborator

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

Left another comment wrt the defined? check. But otherwise looks good to me 👍

@solnic solnic force-pushed the solnic/2124-try-vernier-for-profiling branch 5 times, most recently from b22bd63 to 97e21ac Compare September 3, 2024 09:09
@sl0thentr0py
Copy link
Member

we need to generalize this logic to have different limits according to different envelope item.type but let's make a separate issue/PR for that.

@sl0thentr0py sl0thentr0py mentioned this pull request Sep 23, 2024
Closed
solnic added 20 commits September 30, 2024 12:26
@solnic
Format stackprof_results.json fixture
04e3527
@solnic
Update CHANGELOG
eaa1486
@solnic
Better spec skipping helper
853cf77
@solnic
Fix flaky spec
c75ffd4 
Seems like StackProf.results is leaking between specs
when it's stubbed via `allow`, so we gotta restub it
in the profiler spec too.
@solnic
Add support for Vernier profiler
18c56a2 
- Introduce `Sentry::Vernier::Profiler`
- Introduce `Sentry.config.profiler` that can be set to a class that
  should be used for profiling. By default it's set to `Sentry::Profiler`
@solnic
Improve profiler helpers coverage
11f2f54
@solnic
Restore rack in Gemfile
287ab62
@solnic
Rename config.profiler -> config.profiler_class
e1cee43
@solnic
Minor style tweak
0bf5868
@solnic
Do not re-raise if Vernier crashes
61f3c4a
@solnic
Restore setting thread id as '0'
ca936c9
@solnic
Simplify Output#stacks
8278c2c
@solnic
Minor style tweak
91d5b6e
@solnic
Remove app_dirs_pattern fallback
1592e36
@solnic
Simplify dumping stacks
1b77807
@solnic
Even better rspec skipper
31e014d
@solnic
Update CHANGELOG
0c10596
@solnic
Add early-return so that Vernier::Profiler won't be defined if vernie…
640691d 
…r is not bundled
@solnic
Fix for codecov
6a76f74
@solnic
Better profiler_class setter in config
0e1fde8
@solnic solnic force-pushed the solnic/2124-try-vernier-for-profiling branch from 282c3c2 to 0e1fde8 Compare September 30, 2024 12:26
@solnic
Copy link
Collaborator Author

solnic commented Sep 30, 2024

@sl0thentr0py alright this is now using the increased envelope profile item limit

@sl0thentr0py
Copy link
Member

omg the profiles look perfect <3

sl0thentr0py
sl0thentr0py approved these changes Oct 4, 2024
View reviewed changes
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.

incredible work!!! so happy to ship this

@sl0thentr0py sl0thentr0py merged commit 8dc3d2d into master Oct 4, 2024
139 of 140 checks passed
@sl0thentr0py sl0thentr0py deleted the solnic/2124-try-vernier-for-profiling branch October 4, 2024 13:17
@solnic
Copy link
Collaborator Author

solnic commented Oct 4, 2024

@sl0thentr0py yessssss! Thanks for the reviews and testing! Let's 🚢 it 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@st0012 st0012 st0012 approved these changes

@sl0thentr0py sl0thentr0py sl0thentr0py approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Try vernier for profiling
3 participants
@solnic @sl0thentr0py @st0012