-
-
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(tracing): Better guarding for performance observer #8872
Conversation
return ( | ||
entry.entryType === 'resource' && | ||
'initiatorType' in entry && | ||
'nextHopProtocol' in entry && |
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.
l: this still doesn't guarantee that nextHopProtocol
is a string - at the same time checking whether the key exists is probably enough
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.
yeah, I figured the same.. I mean these are browser APIs, I think if this exists it will be a string, this is more to make sure this is not some other type of resource... IMHO we can do
- Just check existence (current code)
- Check existence +
typeof entry.nextHopProtocol === 'string
- Only check type - requires type cast (which is fine I guess)
typeof (entry as PerformanceResourceTiming).nextHopProtocol === 'string
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 don't mind either way
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 catch!
size-limit report 📦
|
This removes a type cast for the performance observer and actually adds some guards to make sure we do not run into cases where a property we expect to exist does not exist.
It seems we sometimes ran into cases where
nextHopProtocol
would beundefined
, not a string, leading to https://github.com/getsentry/sentry-javascript/blob/develop/packages/tracing-internal/src/browser/request.ts#L202 failing.I now specifically check for the existence of this property, as well as also adding a default for all the time based stuff (0) to ensure these also work in the case one of the fields does not exist (instead of checking for existence of all of them).
Closes #8870
Closes #8863