Skip to content
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(replay): Add BODY_PARSE_ERROR warning & time out fetch response load #9622

Merged
merged 2 commits into from
Nov 22, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Nov 21, 2023

This is a tricky one 😬 Basically, it is possible that fetch returns a readable stream that is ongoing. So if we do await response.text() this will be pending forever, if a stream is ongoing.

I haven't found a way to really check that is the case and avoid parsing this at all 🤔 So the best I could come up with was to instead add a timeout of 500ms when we stop waiting for the fetch body.

This should at least unblock waiting on this, but it will still mean that the response continues to be parsed client-side - I don't think there is a way to abort this 🤔

Additionally, this also refactors this a bit so we add a new BODY_PARSE_ERROR meta warning if the parsing of the body fails, for whatever reason. we may also use this in the replay UI cc @ryan953 somehow?

"fixes" #9616

@mydea mydea requested review from billyvg and c298lee November 21, 2023 13:16
@mydea mydea self-assigned this Nov 21, 2023
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 65.93 KB (+0.49% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 56.01 KB (+0.52% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.01 KB (0%)
@sentry/browser - Webpack (gzipped) 21.32 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 62.34 KB (+0.49% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 29.13 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped) 21.26 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 196.56 KB (+0.44% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 88.38 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 63.34 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.85 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 66.23 KB (+0.48% 🔺)
@sentry/react - Webpack (gzipped) 21.36 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 82.98 KB (+0.4% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 48.15 KB (0%)
@sentry-internal/feedback - Webpack (gzipped) 16.12 KB (0%)

@yjhtry
Copy link

yjhtry commented Nov 22, 2023

@mydea Thank you very much for solving my problem in such a short time. After local testing, the network resource leakage problem was perfectly solved.😊

Finally, a little suggestion, I still hope to get bodySize when responseBodySize is undefined. 😊

code

 const size =
      bodyText && bodyText.length && responseBodySize === undefined
        ? getBodySize(bodyText, textEncoder)
        : responseBodySize;

@mydea
Copy link
Member Author

mydea commented Nov 22, 2023

@mydea Thank you very much for solving my problem in such a short time. After local testing, the network resource leakage problem was perfectly solved.😊

Finally, a little suggestion, I still hope to get bodySize when responseBodySize is undefined. 😊

code

 const size =
      bodyText && bodyText.length && responseBodySize === undefined
        ? getBodySize(bodyText, textEncoder)
        : responseBodySize;

I fear it is impossible to get the body size if no header is set AND we can't parse the body 😬 It'll be undefined / unset in this case 😢

@mydea mydea merged commit 68bb820 into develop Nov 22, 2023
81 checks passed
@mydea mydea deleted the fn/replay-network-body-error branch November 22, 2023 08:43
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