Skip to content

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Sep 24, 2025

Our previous resource timing implementation had two flaws:

  • we'd convert undefined resoure timing values to 0, leading to them being converted the absolute timeOrigin timestamp
    • this would happen if we read performance entries from browsers that don't support the respective timing information
  • we'd convert 0 resource timing values to the absolue timeOrigin timestamp
    • this happens frequently for requests made to cross-origin sources that don't respond with a matching Timing-Allow-Origin header, since a lot of the detailed timing information is guarded by browsers.

This PR

  • drops any timing attributes where the original value was undefined
  • sends 0 for any timing values that were originally 0 (i.e. no longer converts them to the timeOrigin absolute timestamp)

closes https://linear.app/getsentry/issue/JS-968/investigate-httprequestfetch-start-after-httprequestresponse-start

@Lms24 Lms24 requested review from a team, AbhiPrasad and s1gr1d and removed request for a team September 24, 2025 10:23
Copy link

linear bot commented Sep 24, 2025

Copy link
Contributor

github-actions bot commented Sep 24, 2025

size-limit report 📦

Path Size % Change Change
@sentry/browser 24.26 kB - -
@sentry/browser - with treeshaking flags 22.77 kB - -
@sentry/browser (incl. Tracing) 40.43 kB +0.08% +32 B 🔺
@sentry/browser (incl. Tracing, Replay) 78.81 kB +0.05% +32 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 68.48 kB +0.05% +33 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 83.48 kB +0.05% +36 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 95.69 kB +0.04% +29 B 🔺
@sentry/browser (incl. Feedback) 40.97 kB - -
@sentry/browser (incl. sendFeedback) 28.91 kB - -
@sentry/browser (incl. FeedbackAsync) 33.84 kB - -
@sentry/react 25.98 kB - -
@sentry/react (incl. Tracing) 42.41 kB +0.09% +34 B 🔺
@sentry/vue 28.78 kB - -
@sentry/vue (incl. Tracing) 42.24 kB +0.08% +33 B 🔺
@sentry/svelte 24.29 kB - -
CDN Bundle 25.77 kB - -
CDN Bundle (incl. Tracing) 40.33 kB +0.08% +31 B 🔺
CDN Bundle (incl. Tracing, Replay) 76.56 kB +0.05% +36 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 82.05 kB +0.05% +35 B 🔺
CDN Bundle - uncompressed 75.37 kB - -
CDN Bundle (incl. Tracing) - uncompressed 119.39 kB +0.09% +99 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 234.51 kB +0.05% +99 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 247.27 kB +0.05% +99 B 🔺
@sentry/nextjs (client) 44.42 kB +0.07% +27 B 🔺
@sentry/sveltekit (client) 40.85 kB +0.09% +33 B 🔺
@sentry/node-core 50.02 kB -0.01% -1 B 🔽
@sentry/node 152.83 kB - -
@sentry/node - without tracing 91.94 kB -0.01% -1 B 🔽
@sentry/aws-serverless 105.39 kB - -

View base workflow run

@Lms24 Lms24 self-assigned this Sep 24, 2025
* In contrast to `dropUndefinedKeys` in core this funciton only works on first-level
* key-value objects and does not recursively go into object properties or arrays.
*/
function dropUndefinedKeysFromObject<T extends object>(attrs: T): T {
Copy link
Member Author

@Lms24 Lms24 Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is probably useful to be extracted into browser or core at some point. I know we had our reservations against dropUndefinedKeys but IIRC this was mainly due to over-usage and it recursively going through the object. I think a light-weight implementation like this one should be fine but happy to hear other opinions.

Main motivation for using it in browser: it's more size-efficient than checking every value for definedness before adding the attribute.

Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, this makes sense to me! We should make sure we handle this appropriately in product, as this could also be confusing to users why this would be 0/

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good change!

Lms24 and others added 2 commits September 24, 2025 15:34
Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
@Lms24 Lms24 enabled auto-merge (squash) September 24, 2025 13:38
@Lms24 Lms24 merged commit 105f78e into develop Sep 24, 2025
147 of 148 checks passed
@Lms24 Lms24 deleted the lms/fix-browser-handle-0-resourceTiming-values branch September 24, 2025 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants