-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(core): Make beforeSend
handling defensive for different event types
#6507
Conversation
const beforeSendProcessor = options[beforeSendProcessorName]; | ||
const isError = !event.type; | ||
const eventType = event.type || 'error'; | ||
const beforeSendLabel = `before send for type \`${eventType}\``; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 becomebeforeSendProfile
which doesn't really exist as of now I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach is fine
size-limit report 📦
|
packages/core/src/baseclient.ts
Outdated
@@ -628,13 +630,14 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> { | |||
} | |||
|
|||
const isTransaction = event.type === 'transaction'; | |||
const beforeSendProcessorName = isTransaction ? 'beforeSendTransaction' : 'beforeSend'; | |||
const beforeSendProcessor = options[beforeSendProcessorName]; | |||
const isError = !event.type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm probably missing something but why not use the new isErrorEvent
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, especially the new processBeforeSend function. Makes things a lot clearer
function isErrorEvent(event: Event): event is ErrorEvent { | ||
return event.type === undefined; | ||
} | ||
|
||
function isTransactionEvent(event: Event): event is TransactionEvent { | ||
return event.type === 'transaction'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random thought: We have this is.ts
file in utills. WDYT, does it make sense to move them there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good change!
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
Our current handling of
beforeSend
is not very defensive, in the sense that it will just usebeforeSend
for 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.