Experimental support for multi-threaded profiling using Vernier

[solnic]

Experimental support for Vernier.

Screenshots

[codecov]

Codecov Report

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

Project coverage is 98.65%. Comparing base (04e030f) to head (282c3c2).

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.65%   -0.03%     
==========================================
  Files         210      217       +7     
  Lines       13957    14307     +350     
==========================================
+ Hits        13773    14115     +342     
- Misses        184      192       +8     

Components	Coverage Δ
sentry-ruby	99.01% <99.25%> (-0.05%)	⬇️
sentry-rails	97.34% <ø> (ø)
sentry-sidekiq	97.07% <ø> (ø)
sentry-resque	96.79% <ø> (ø)
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 2 files with indirect coverage changes

[sl0thentr0py]

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

will do a detailed test round later in the week

[st0012]

Is it possible for this part being loaded but Vernier is still not defined?

[solnic]

@st0012 yes, we rescue from LoadError if vernier is not bundled. It's the same with stackprof, its Profiler gets required even if stackprof is not available (see transaction.rb). I think we can make this whole conditional code loading better in the future and limit the amount of files that sentry requires. ie we can have some API for configuring features and then based on its configuration a smart code loader would know what to do.

[st0012]

I think we can return in the rescue block then we won't need to call defined? at all. For example:

begin
  require "foo"
rescue LoadError
  return
end

Foo.bar

[sl0thentr0py]

sorry to come back to this, but can we keep both similar? Right now stackprof has one way and vernier has another.

[solnic]

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

[st0012]

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

[sl0thentr0py]

@solnic we're not requiring the new sentry/vernier/profiler anywhere? We should not force end users to require another obscure path from within the gem, they should all be setup internally.

I would also prefer a warning message in Configuration when people try to set the profiler to vernier but vernier is not installed as a peer dependency gem.

[sl0thentr0py]

I tried on a simple rails controller with some database calls and the profile is not sent due to being oversized.
However, the size limits for a profile payload are much larger (50MB) than for errors/transactions (1MB), so we need to add this logic in Envelope::Item.

[solnic]

@solnic we're not requiring the new sentry/vernier/profiler anywhere? We should not force end users to require another obscure path from within the gem, they should all be setup internally.

I would also prefer a warning message in Configuration when people try to set the profiler to vernier but vernier is not installed as a peer dependency gem.

@sl0thentr0py OK I made it so that sentry/vernier/profiler is required automatically and if you try to set it in the config when vernier is not available, then you'll get a meaningful error message:

3.1.6 :002 > Sentry::Configuration.new.tap { |c| c.profiler_class = Sentry::Vernier::Profiler }
/workspaces/sentry/sentry-ruby/sentry-ruby/lib/sentry/configuration.rb:506:in `rescue in profiler_class=': Please add the 'vernier' gem to your Gemfile to use the Vernier profiler with Sentry. (ArgumentError)
        from /workspaces/sentry/sentry-ruby/sentry-ruby/lib/sentry/configuration.rb:503:in `profiler_class='
        from (irb):2:in `block in <top (required)>'
        from <internal:kernel>:90:in `tap'
        from (irb):2:in `<main>'

[solnic]

I tried on a simple rails controller with some database calls and the profile is not sent due to being oversized.
However, the size limits for a profile payload are much larger (50MB) than for errors/transactions (1MB), so we need to add this logic in Envelope::Item.

@sl0thentr0py wdym by "this logic" exactly?

[sl0thentr0py]

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.