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(integrations): Ensure httpclient integration works with Request #7786

Merged
merged 3 commits into from Apr 7, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Apr 7, 2023

We cannot clone a request if it has a body which was read (which seems to happen somewhere). So instead we just re-use the same request if possible, instead of creating a new one.

This fixes this in two places:

  1. Make sure we only ever keep a clone of the request in the fetch instrumentation args in the first place.
  2. If, for whatever reason, this is still read somewhere (e.g. by some other integration, ...), we make sure to handle this in the httpclient integration, and avoid creating unnecessary copies of the Request where we don't need it.

I added some tests for different scenarios covering this.

Fixes #7785

We cannot clone a request if it has a body which was read (which seems to happen somewhere). So instead we just re-use the same request if possible, instead of creating a new one.
@mydea mydea added the Type: Bug label Apr 7, 2023
@mydea mydea requested review from lforst and AbhiPrasad April 7, 2023 08:50
@mydea mydea self-assigned this Apr 7, 2023
@@ -14,7 +14,7 @@
"build:dev": "yarn build",
"build:watch": "run-p build:transpile:watch build:types:watch",
"build:dev:watch": "run-p build:watch",
"build:transpile:watch": "yarn build:rollup --watch",
Copy link
Member

Choose a reason for hiding this comment

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

What is this about?

Copy link
Member Author

Choose a reason for hiding this comment

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

just noticed this while working here that this is actually referencing wrong commands, so fixed them while at it! same for integration-shims, copy paste error 😅

@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.92 KB (+0.01% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 65.42 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 19.48 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified) 57.86 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 21.1 KB (0%)
@sentry/browser - Webpack (minified) 68.85 KB (0%)
@sentry/react - Webpack (gzipped + minified) 21.12 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.88 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 28.48 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 26.72 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 44.92 KB (+0.01% 🔺)
@sentry/replay - Webpack (gzipped + minified) 38.87 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 63.81 KB (0%)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 56.84 KB (0%)

@mydea
Copy link
Member Author

mydea commented Apr 7, 2023

OK, turns out we can't do (all of) this - we actually use the passed in request and add headers to it in browser tracing, so we cannot swap this with a clone. So I'll only do the change for the httpclient integration, which should be fine too.

@mydea mydea merged commit c3a42f3 into develop Apr 7, 2023
59 checks passed
@mydea mydea deleted the fn/httpclient-request branch April 7, 2023 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpClient: Cannot construct a Request with a Request object that has already been used.
3 participants