fix(core): Make beforeSend handling defensive for different event types#6507
Merged
fix(core): Make beforeSend handling defensive for different event types#6507
beforeSend handling defensive for different event types#6507Conversation
mydea
commented
Dec 13, 2022
| const beforeSendProcessor = options[beforeSendProcessorName]; | ||
| const isError = !event.type; | ||
| const eventType = event.type || 'error'; | ||
| const beforeSendLabel = `before send for type \`${eventType}\``; |
Member
Author
There was a problem hiding this comment.
Not completely sure about this... I figured this is the most "compact" solution. Other options:
- Make a map - but how to handle "unknown" beforeSend stuff (e.g. if
type === 'profile', what should this be? - Auto-build this like
beforeSend${event.type ? capitzalize(event.type) : ''}- same issue as before, this would becomebeforeSendProfilewhich doesn't really exist as of now I guess.
Contributor
size-limit report 📦
|
mydea
commented
Dec 13, 2022
packages/core/src/baseclient.ts
Outdated
| const isTransaction = event.type === 'transaction'; | ||
| const beforeSendProcessorName = isTransaction ? 'beforeSendTransaction' : 'beforeSend'; | ||
| const beforeSendProcessor = options[beforeSendProcessorName]; | ||
| const isError = !event.type; |
Member
Author
There was a problem hiding this comment.
The alternative implementation would be IMHO:
if (!isTransaction && !isError) {
// skip...
}This would probably by smaller in footprint, but we may need to revisit this if/when we also want these hooks for e.g. replay or profiles.
Member
There was a problem hiding this comment.
hmm probably missing something but why not use the new isErrorEvent function?
lforst
reviewed
Dec 13, 2022
lforst
reviewed
Dec 13, 2022
lforst
reviewed
Dec 13, 2022
lforst
approved these changes
Dec 13, 2022
Lms24
approved these changes
Dec 13, 2022
Member
Lms24
left a comment
There was a problem hiding this comment.
I like it, especially the new processBeforeSend function. Makes things a lot clearer
Lms24
reviewed
Dec 13, 2022
Comment on lines
+827
to
+833
| function isErrorEvent(event: Event): event is ErrorEvent { | ||
| return event.type === undefined; | ||
| } | ||
|
|
||
| function isTransactionEvent(event: Event): event is TransactionEvent { | ||
| return event.type === 'transaction'; | ||
| } |
Member
There was a problem hiding this comment.
Random thought: We have this is.ts file in utills. WDYT, does it make sense to move them there?
Member
Author
There was a problem hiding this comment.
hmm, not sure, that could be confusing, as there it is a lot about built ins etc. there is actually also this in there:
export function isErrorEvent(wat: unknown): boolean {
return isBuiltin(wat, 'ErrorEvent');
}but that checks something else I guess 😅
d170dbf to
2a95f58
Compare
Make sure that we do not call `beforeSend` for non-error events, and that we only sample error events.
2a95f58 to
50d837e
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Our current handling of
beforeSendis not very defensive, in the sense that it will just usebeforeSendfor anything that is not a transaction. So e.g. for profiles, or replays, if they ever go through this code path (which replay currently isn't, but may in the near future, not sure about profiles), this is subject to break subtly.This PR refactors this to be more defensive, by explicitly calling the respective callback. While at it, I also narrowed the type for the handlers a bit.