Skip to content

SDK should drop the event when any event processor returns nil#1523

Merged
st0012 merged 3 commits intomasterfrom
fix-event-processor
Aug 9, 2021
Merged

SDK should drop the event when any event processor returns nil#1523
st0012 merged 3 commits intomasterfrom
fix-event-processor

Conversation

@st0012
Copy link
Copy Markdown
Contributor

@st0012 st0012 commented Aug 4, 2021 •
edited
Loading

This is something we should've supported but was missed.

@st0012 st0012 added this to the 4.7.0 milestone Aug 4, 2021
@st0012 st0012 self-assigned this Aug 4, 2021
@st0012 st0012 requested a review from rhcarvalho August 4, 2021 09:10
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 4, 2021 •
edited
Loading

Codecov Report

Merging #1523 (f8e7414) into master (d050286) will increase coverage by 0.55%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1523      +/-   ##
==========================================
+ Coverage   98.21%   98.76%   +0.55%     
==========================================
  Files         218      123      -95     
  Lines       10616     6789    -3827     
==========================================
- Hits        10426     6705    -3721     
+ Misses        190       84     -106
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/client.rb 97.46% <100.00%> (+0.09%) ⬆️
...ntry-ruby/spec/sentry/client/event_sending_spec.rb 99.53% <100.00%> (+0.01%) ⬆️
...try-raven/lib/sentry_raven_without_integrations.rb
...aven/integrations/rails/controller_methods_spec.rb
...try-raven/lib/raven/interfaces/single_exception.rb
sentry-raven/lib/raven/utils/real_ip.rb
sentry-raven/spec/raven/integrations/rake_spec.rb
...try-raven/lib/sentry-raven-without-integrations.rb
sentry-raven/spec/raven/integrations/rack_spec.rb
sentry-raven/lib/raven/integrations/sidekiq.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 d050286...f8e7414. Read the comment docs.

@st0012 st0012 mentioned this pull request Aug 4, 2021
Closed
@st0012
Copy link
Copy Markdown
Contributor Author

st0012 commented Aug 6, 2021

@rhcarvalho can you give this a quick look today? thanks 😄

@st0012 st0012 merged commit a1f1dbc into master Aug 9, 2021
@st0012 st0012 deleted the fix-event-processor branch August 9, 2021 14:23
@st0012 st0012 modified the milestones: 4.7.0, 4.6.5 Aug 12, 2021
rhcarvalho
rhcarvalho reviewed Aug 12, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

Late LGTM 👍

By the way, is the SDK implementing the same behavior for global event processors and beforeSend?

@st0012
Copy link
Copy Markdown
Contributor Author

st0012 commented Aug 13, 2021

What're global event processors? I thought they're always attached to a scope?

It's implemented with before_send though.

According to the documentation, there are 5 steps in the event pipeline:

  1. If the SDK is disabled, Sentry discards the event right away.
  2. The client samples events as defined by the configured sample rate. Events may be discarded randomly, according to the sample rate.
  3. The scope is applied, using apply_to_event. The scope’s event processors are invoked in order.
  4. Sentry invokes the before-send hook.
  5. Sentry passes the event to the configured transport. The transport can discard the event if it does not have a valid DSN; its internal queue is full; or due to rate limiting, as requested by the server.

4 Is already implemented and this PR fixes 3.

@rhcarvalho
Copy link
Copy Markdown
Contributor

rhcarvalho commented Aug 13, 2021

Some SDKs have, in addition to event processors added to Scope, event processors on Client and global. Those event processors are the same type of functions as beforeSend, the difference being that there can be multiple of them.

Most integrations use event processors to modify or drop events.

If the Ruby SDK doesn't have other event processors, that's okay and I suggest keeping things simple and not adding anything more.

My question was in the sense to make sure we fixed all similar issues related to the behavior of dropping events, it seems we did. Thanks! 🙏

@st0012
Copy link
Copy Markdown
Contributor Author

st0012 commented Aug 13, 2021

I see. Thanks for explaining 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@rhcarvalho rhcarvalho rhcarvalho left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@st0012 st0012

Labels

sentry-ruby

Projects

None yet

Milestone

4.6.5

Development

Successfully merging this pull request may close these issues.

3 participants

@st0012 @codecov-commenter @rhcarvalho