Change ordering of event drop mechanisms #1390
Conversation
As requested by @mitsuhiko this PR shall serve as basis for discussing the ordering of event drop mechanisms and its implications. We are planning for `sample_rate` to update the session counts despite dropping an event (see getsentry/develop#551 and getsentry/develop#537). Without changing the order of filtering mechanisms this would mean any event dropped by `sample_rate` would update the session even if it would be dropped by `ignore_errors` which should not update the session counts when dropping an event. By changing the order we would first drop `ignored_errors` and only then check `sample_rate`, so session counts would not be affected in the case mentioned before. The same reasoning could probably be applied to `event_processor` and `before_send` but we don't know why a developer decided to drop an event there. Was it because they don't care about the event (then session should not be updated) or to save quota (session should be updated)? Also these may be more expensive in terms of performance (developers can provide their own implementations for both of those on some SDKs). So moving them before `sample_rate` would execute `before_send` and `event_processor` for every event instead of only doing it for the sampled events.
|
I did some refactoring to make the code better readable. Here a tl:dr of what the code change does: Dropping events decision before:
Now changed to dropping events decision like this:
After events where dropped (or not dropped) the sessions are counted here: The idea (not yet reflected in code) is now that we do not do the dropping first for all events and then update the sessions. But do the session update right before an event is dropped by sample_rate. So that all events that are not dropped are counted in sessions and also events that where dropped because of the sample rate are counted in the sessions. |
|
@antonpirker the way I understood what we're planning to do is different from what you describe here:
Events dropped by |
|
Ok, so in the So if events are dropped in So the solution would be to move the "sample rate dropping code" out of This way we do all the "dropping events" stuff first (based on Is this the behavior this should have? |
|
The initial consensus only specified moving Things to consider:
|
|
Ok. Now I got it. Thanks for clarification. So I think we can safely deploy this change here because it does not break anything. I think we should discuss |
We have already merged #1390 where we moved `ignore_errors` before sampling. Background for the order change: We want to update the session for dropped events in case the event is dropped by sampling. Events dropped by other mechanisms should not update the session. See getsentry/develop#551 Now we would like to discuss if we can simply move sampling after `before_send` and `event_processor` and update the session right before sampling. What are implications of changing this? * How does this affect session count and session crash rate? * Will this have a negative effect on performance as `before_send` and `event_processor` will now be executed for every event instead of only being executed for sampled events? Developers may have fine tuned their sample rate for a good performance tradeoff and now we change. Also developers can supply their own implementations for both `before_send` and `event_processor` on some SDKs so we have no way of predicting performance I'm afraid. * We are uncertain why a developer chose to drop an event in `before_send` and `event_processor`: ** Was it because they want to ignore the event - then it shouldn't update the session ** Or was it to save quota - then it should update the session Please feel free to optimize the code this is just to start the discussion.
Python now serves as reference implementations. We've recently changed the order there, see getsentry/sentry-python#1394 and getsentry/sentry-python#1390
* Document session update for dropped events See #537 and getsentry/sentry-java#1916 Do we also want to document order of the filtering mechanisms? If so do we go with the python implementation order as a template? * Add filter order Python now serves as reference implementations. We've recently changed the order there, see getsentry/sentry-python#1394 and getsentry/sentry-python#1390 * Session update should be sent... ... despite the event being dropped in application mode for certain cases.
* Document session update for dropped events See #537 and getsentry/sentry-java#1916 Do we also want to document order of the filtering mechanisms? If so do we go with the python implementation order as a template? * Add filter order Python now serves as reference implementations. We've recently changed the order there, see getsentry/sentry-python#1394 and getsentry/sentry-python#1390 * Session update should be sent... ... despite the event being dropped in application mode for certain cases.
As requested by @mitsuhiko this PR shall serve as basis for discussing the ordering of event drop mechanisms and its implications.
We are planning for
sample_rateto update the session counts despite dropping an event (see getsentry/develop#551 and getsentry/develop#537). Without changing the order of filtering mechanisms this would mean any event dropped bysample_ratewould update the session even if it would be dropped byignore_errorswhich should not update the session counts when dropping an event. By changing the order we would first dropignored_errorsand only then checksample_rate, so session counts would not be affected in the case mentioned before. The same reasoning could probably be applied toevent_processorandbefore_sendbut we don't know why a developer decided to drop an event there. Was it because they don't care about the event (then session should not be updated) or to save quota (session should be updated)? Also these may be more expensive in terms of performance (developers can provide their own implementations for both of those on some SDKs). So moving them beforesample_ratewould executebefore_sendandevent_processorfor every event instead of only doing it for the sampled events.