search

getsentry / sentry-ruby

Sentry SDK for Ruby
https://sentry.io/for/ruby
MIT License
933 stars 494 forks source link

Improvements around the cron monitoring feature #2212

Closed st0012 closed 10 months ago

st0012 commented 10 months ago

After reviewing the recent cron monitoring PRs, I want to make a few small improvements:

  1. Testing the MonitorCheckIns module end-to-end: Since the module captures the core logic of the cron monitoring feature, I think making sure it works e2e in tests is very beneficial, especially when the test complexity doesn't really increase much (see the changes).
  2. Applying the global cron configs in the MonitorCheckIns: this will keep the MonitorConfig simple and isolated.
  3. Fixing MonitorCheckIns#perform's rescue scope: it should only be applied AFTER the start local is defined. Otherwise, Sentry.utc_now.to_i - start would cause a new TypeError and overshadow the original exception.
  4. There's an unnecessary extend

skip-changelog

codecov[bot] commented 10 months ago

Codecov Report

Merging #2212 (89373d3) into master (babdd55) will increase coverage by 30.91%. The diff coverage is 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2212 +/- ## =========================================== + Coverage 66.48% 97.40% +30.91% =========================================== Files 100 102 +2 Lines 3742 3819 +77 =========================================== + Hits 2488 3720 +1232 + Misses 1254 99 -1155 ``` | [Components](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2212/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | Coverage Δ | | |---|---|---| | [sentry-ruby](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2212/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `98.08% <100.00%> (+42.17%)` | :arrow_up: | | [sentry-rails](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2212/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `94.98% <ø> (ø)` | | | [sentry-sidekiq](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2212/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `94.70% <ø> (ø)` | | | [sentry-resque](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2212/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `93.65% <ø> (+1.58%)` | :arrow_up: | | [sentry-delayed_job](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2212/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `94.44% <ø> (ø)` | | | [sentry-opentelemetry](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2212/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `100.00% <ø> (ø)` | | | [Files](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2212?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | Coverage Δ | | |---|---|---| | [sentry-ruby/lib/sentry/cron/monitor\_check\_ins.rb](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2212?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-c2VudHJ5LXJ1YnkvbGliL3NlbnRyeS9jcm9uL21vbml0b3JfY2hlY2tfaW5zLnJi) | `100.00% <100.00%> (+61.29%)` | :arrow_up: | | [sentry-ruby/lib/sentry/cron/monitor\_config.rb](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2212?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-c2VudHJ5LXJ1YnkvbGliL3NlbnRyeS9jcm9uL21vbml0b3JfY29uZmlnLnJi) | `100.00% <100.00%> (+40.00%)` | :arrow_up: | ... and [54 files with indirect coverage changes](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2212/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry)
st0012 commented 10 months ago

@sl0thentr0py This also fixes an order-dependent CI failure in master, which can be reproduced with:

rspec './spec/sentry/client_spec.rb[1:7:2,1:7:3]' './spec/sentry/cron/monitor_config_spec.rb[1:1:1]' --seed 60911

It happens because the current monitor_config_spec sets Sentry.configuration.cron, but the value would be picked up by client specs as they don't initialise the SDK themselves.