Skip to content
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.

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

Don't initialize Event objects when they won't be sent #1687

Merged
merged 2 commits into from
Jan 14, 2022
Merged

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Jan 14, 2022

Fixes #1683

@st0012 st0012 added this to the 4.9.1 milestone Jan 14, 2022
@st0012 st0012 added this to In progress in 4.x via automation Jan 14, 2022
@st0012 st0012 self-assigned this Jan 14, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2022

Codecov Report

Merging #1687 (a63511f) into master (7697af6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1687   +/-   ##
=======================================
  Coverage   98.55%   98.55%           
=======================================
  Files         136      136           
  Lines        7691     7695    +4     
=======================================
+ Hits         7580     7584    +4     
  Misses        111      111           
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/client.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/hub.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry_spec.rb 99.74% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7697af6...a63511f. Read the comment docs.

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.

hmm ideally this check should be done just once in a centralized place, but I can't see a good place for it without changing too many things, so it's fine for now :)

@st0012
Copy link
Collaborator Author

st0012 commented Jan 14, 2022

@sl0thentr0py Yeah I agree with you. I think it depends on whether we expect users to initialize events themselves and call capture_event manually.

If we do, then I think the current approach (2 levels of checks) is necessary.
If we don't, we need to prohibit such usages, which should have its own PR.

@sl0thentr0py
Copy link
Member

Event.new should be considered as internal api, users shouldn't really use it directly. Only methods on hub or those exposed directly on the top-level Sentry module are our 'contract'.

@st0012 st0012 merged commit 4e840e6 into master Jan 14, 2022
4.x automation moved this from In progress to Done Jan 14, 2022
@st0012 st0012 deleted the fix-#1683 branch January 14, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4.x
  
Done
Development

Successfully merging this pull request may close these issues.

Sentry has RuntimeError when not enabled in current environment
3 participants