Skip to content

fix#1246: raise error when send_event failed - #1268

Closed
sujh wants to merge 1 commit into
getsentry:masterfrom
sujh:fix#1246
Closed

fix#1246: raise error when send_event failed#1268
sujh wants to merge 1 commit into
getsentry:masterfrom
sujh:fix#1246

Conversation

@sujh

@sujh sujh commented Feb 5, 2021

Copy link
Copy Markdown

Description

Sentry::Transport will swallow exceptions when send_event failed, so the app include sidekiq can't catch error and retry.
Related issue: https://github.com/getsentry/sentry-ruby/issues/1246

@codecov-io

codecov-io commented Feb 5, 2021

Copy link
Copy Markdown

Codecov Report

Merging #1268 (b13b122) into master (71fd7ca) will increase coverage by 0.60%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1268      +/-   ##
==========================================
+ Coverage   97.99%   98.59%   +0.60%     
==========================================
  Files         200      105      -95     
  Lines        8667     4836    -3831     
==========================================
- Hits         8493     4768    -3725     
+ Misses        174       68     -106     
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/transport.rb 97.91% <100.00%> (ø)
sentry-ruby/spec/sentry/transport_spec.rb 100.00% <100.00%> (ø)
sentry-raven/spec/raven/transports/stdout_spec.rb
sentry-raven/spec/raven/linecache_spec.rb
sentry-raven/lib/raven/instance.rb
...ntry-raven/lib/raven/processor/removestacktrace.rb
sentry-raven/lib/raven/linecache.rb
...ven/spec/raven/utils/exception_cause_chain_spec.rb
...-raven/spec/raven/integrations/delayed_job_spec.rb
sentry-raven/lib/raven/configuration.rb
... and 87 more

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 71fd7ca...b13b122. Read the comment docs.

@st0012

st0012 commented Feb 15, 2021

Copy link
Copy Markdown
Contributor

@sujh thanks for the PR! I agree that send_event should raise exceptions when it failed. but that means Sentry.capture_* would also fail if there's a networking issue. so we need to consider 2 cases:

  1. when send_event is used alone, perhaps in a background job, it should raise exceptions when failed.
  2. when send_event is called directly from the chain of Sentry.capture_* calls, it should not raise exceptions.

I'm actually working on a solution right now and will open a PR later this week 🙂

@sujh

sujh commented Feb 16, 2021

Copy link
Copy Markdown
Author

@st0012 glad to know the progress! I am kinda confused why Sentry.capture_* shouldn't raise error. If there is an error in Sentry.capture_*, it will be swallowed by ThreadPoolExecutor.

@st0012

st0012 commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

@sujh although events are sent by workers by default now, there still are users who fall back to the old behavior by setting

config.background_worker_threads = 0

in those cases, the error will be raised in the same thread too. so we also need to maintain the same behavior for them.

@sujh

sujh commented Feb 16, 2021

Copy link
Copy Markdown
Author

@st0012 Oh, got it. Thanks for your patiant explain🙂

@st0012

st0012 commented Feb 17, 2021

Copy link
Copy Markdown
Contributor

@sujh I just added #1290 to provide a guideline on exception handling and list all the scenarios I can come up with. It'd be great if you can give it a look and provide your opinions on it 🙂

@st0012

st0012 commented Mar 7, 2021

Copy link
Copy Markdown
Contributor

closing because the functionality has been implemented in #1298

@st0012 st0012 closed this Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants