Update session also for non sampled events and change filter order #1394
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
made it a draft so we don't merge before we're sure of the changes |
|
@adinauer made a fix because the new sample function should only affect errors and not transactions |
sl0thentr0py
approved these changes
Apr 20, 2022
adinauer
added a commit
to getsentry/develop
that referenced
this pull request
Apr 20, 2022
Python now serves as reference implementations. We've recently changed the order there, see getsentry/sentry-python#1394 and getsentry/sentry-python#1390
4 tasks
adinauer
added a commit
to getsentry/develop
that referenced
this pull request
Apr 26, 2022
* 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.
brustolin
pushed a commit
to getsentry/develop
that referenced
this pull request
Apr 26, 2022
* 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have already merged #1390 where we moved
ignore_errorsbefore 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_sendandevent_processorthen update the session right before sampling.What are implications of changing this?
before_sendandevent_processorwill 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 how this behaves. Also developers can supply their own implementations for bothbefore_sendandevent_processoron some SDKs so we have no way of predicting performance I'm afraid.before_sendandevent_processor:Please feel free to optimize the code this is just to start the discussion.