Skip to content

Introduce Configuration#validate#2537

Merged
solnic merged 7 commits intomasterfrom
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 intomasterfrom
solnic/2534-i-get-a-message-to-include-the-stackprof-gem-while-im-using-vernier-for-profiling

Conversation

@solnic
Copy link
Copy Markdown
Collaborator

@solnic solnic commented Feb 3, 2025

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

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 3, 2025

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
Copy link
Copy Markdown
Collaborator Author

solnic commented Feb 3, 2025

@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
Copy link
Copy Markdown
Member

yes sure, this is definitely an improvement

@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
Copy link
Copy Markdown
Collaborator Author

solnic commented Feb 4, 2025

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

Copy link
Copy Markdown
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.

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

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

2 participants