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

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

Merged
merged 3 commits into from
Aug 9, 2021

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Aug 4, 2021

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 added this to In progress in 4.x via automation Aug 4, 2021
@st0012 st0012 requested a review from rhcarvalho August 4, 2021 09:10
@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2021

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
Copy link
Collaborator 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
4.x automation moved this from In progress to Done Aug 9, 2021
@st0012 st0012 modified the milestones: 4.7.0, 4.6.5 Aug 12, 2021
Copy link
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
Collaborator 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
Contributor

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
Collaborator 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
Projects
4.x
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants