Skip to content

Introduce Configuration#validate#2537

Merged
solnic merged 7 commits into
masterfrom
solnic/2534-i-get-a-message-to-include-the-stackprof-gem-while-im-using-vernier-for-profiling
Feb 4, 2025
Merged

Introduce Configuration#validate#2537
solnic merged 7 commits into
masterfrom
solnic/2534-i-get-a-message-to-include-the-stackprof-gem-while-im-using-vernier-for-profiling

Conversation

@solnic

@solnic solnic commented Feb 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This moves checking profiler setup from configuration writers to a new method called validate that is called automatically in Sentry.init after its done setting up configuration instance.

There's also a handy new method for defining simple validations for Configuration:

module Sentry
  class Configuration
    # ...

    validate :traces_sample_rate, optional: true, type: :numeric
    validate :profiles_sample_rate, optional: true, type: :numeric

    # ...
  end
end

The benefit here is that we can validate values that actually need other values in order to be validated, or to perform additional checks like if a 3rd-party dependency is available.

Previously it was not possible because in the moment of validation the configuration was not completed.

Closes #2534

@solnic solnic linked an issue Feb 3, 2025 that may be closed by this pull request
I get a message to include the 'stackprof' gem while I'm using 'vernier' for profiling #2534
Closed
@codecov

codecov Bot commented Feb 3, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 78.57143% with 6 lines in your changes missing coverage. Please review.

Project coverage is 69.84%. Comparing base (1ee671c) to head (eece6a1).
Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
sentry-ruby/lib/sentry/configuration.rb 77.77% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2537      +/-   ##
==========================================
+ Coverage   69.32%   69.84%   +0.51%     
==========================================
  Files         125      126       +1     
  Lines        4721     4725       +4     
==========================================
+ Hits         3273     3300      +27     
+ Misses       1448     1425      -23
Components Coverage Δ
sentry-ruby 60.78% <66.66%> (+0.78%) ⬆️
sentry-rails 97.48% <100.00%> (+0.71%) ⬆️
sentry-sidekiq 95.52% <100.00%> (-1.56%) ⬇️
sentry-resque 92.64% <ø> (-0.22%) ⬇️
sentry-delayed_job 95.65% <ø> (ø)
sentry-opentelemetry 100.00% <ø> (+0.68%) ⬆️
Files with missing lines Coverage Δ
sentry-ruby/lib/sentry-ruby.rb 67.71% <100.00%> (+0.14%) ⬆️
sentry-ruby/lib/sentry/configuration.rb 85.86% <77.77%> (-0.02%) ⬇️

... and 24 files with indirect coverage changes

@solnic

solnic commented Feb 3, 2025

Copy link
Copy Markdown
Collaborator Author

@sl0thentr0py @st0012 what do you think about this approach? If you could give me a 👍🏻 I'd love to continue and make a tiny API for defining validations for our settings so that we can have it work consistently for all values. Seems like a nice improvement and it'd fix the issue we're facing with profiler setup.

@sl0thentr0py

sl0thentr0py commented Feb 3, 2025

Copy link
Copy Markdown
Member

yes sure, this is definitely an improvement

solnic
solnic commented Feb 4, 2025
View reviewed changes
Comment thread sentry-ruby/lib/sentry/configuration.rb
@solnic
solnic marked this pull request as ready for review February 4, 2025 10:51
@solnic
solnic requested review from sl0thentr0py and st0012 February 4, 2025 10:52
@solnic

solnic commented Feb 4, 2025

Copy link
Copy Markdown
Collaborator Author

Please ignore jruby failures, currently jruby doesn't bundle on our CI. There's #2536 pending PR that's meant to fix it.

sl0thentr0py
sl0thentr0py approved these changes Feb 4, 2025
View reviewed changes

@sl0thentr0py sl0thentr0py left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's ignore jruby ci for now, I'm fixing that separately

@solnic
solnic merged commit b5a4948 into master Feb 4, 2025
@solnic
solnic deleted the solnic/2534-i-get-a-message-to-include-the-stackprof-gem-while-im-using-vernier-for-profiling branch February 4, 2025 12:27
@solnic solnic mentioned this pull request Feb 5, 2025
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Jun 25, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sl0thentr0py sl0thentr0py sl0thentr0py approved these changes
@st0012 st0012 Awaiting requested review from st0012

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

I get a message to include the 'stackprof' gem while I'm using 'vernier' for profiling

2 participants

@solnic @sl0thentr0py