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(remix): Clone erroneous responses not to consume their body streams. #5429

Merged
merged 1 commit into from Jul 21, 2022

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Jul 18, 2022

Fixes: #5423

Ref: #5405

We were extracting the bodies of 4xx/5xx responses to capture, but Remix also uses them, we should not consume their body streams, as they are only available once in response lifespans.

This PR updates extractData to work on a clone response.

@onurtemizkan
Copy link
Collaborator Author

onurtemizkan commented Jul 18, 2022

Converted this to a draft, as it's not completely fixing #5425 (reporting an empty object for a simple 404 response with an empty body).

A question for the team:

Would it be reasonable to only capture 5xx responses instead? It seems users are not expecting a 4xx on their dashboard.

Or alternatively, maybe capturing only if there is a non-empty body inside the 4xx response?

@alexblack
Copy link

alexblack commented Jul 18, 2022

As a user I definitely wouldn't want 404 pages (not found) showing up in Sentry. However, I would want 400 errors (bad request) showing up in Sentry. However, if my loader has thrown a Response with status == 400 then I've likely handled an exception already...? (Same for status == 500)

@lobsterkatie
Copy link
Member

lobsterkatie commented Jul 19, 2022

Would it be reasonable to only capture 5xx responses instead? It seems users are not expecting a 4xx on their dashboard.

This would match what we do in other SDKs. Here's that check in the nextjs SDK, for example:

// 404s (and other 400-y friends) can trigger `_error`, but we don't want to send them to Sentry
const statusCode = (res && res.statusCode) || contextOrProps.statusCode;
if (statusCode && statusCode < 500) {
return Promise.resolve();
}

We do tell folks in the docs how to catch 404s (on the front end). Maybe we should add a snippet for catching other 4xx errors, front and/or back end? Or maybe make it configurable on the backend? Regardless, IMHO that's a separate question from this one. Let's get this body fix in, make another PR to restrict it to 5xx, and take up the 4xx question separately.

@onurtemizkan onurtemizkan marked this pull request as ready for review July 20, 2022 07:57
@lobsterkatie lobsterkatie merged commit a60edf8 into master Jul 21, 2022
@lobsterkatie lobsterkatie deleted the onur/remix-clone-response branch July 21, 2022 18:52
lobsterkatie pushed a commit that referenced this pull request Jul 21, 2022
Fixes: #5425 

Ref: #5429, #5405 

As per review #5429 (comment), we need to define how and when we should capture a 4xx error.

We will only capture thrown 5xx responses until then.
GJZwiers added a commit to GJZwiers/sentry_deno that referenced this pull request Jul 23, 2022
* fix(remix): Use cjs for main entry point (#5352)

* ref(tracing): Only add `user_id` to DSC if `sendDefaultPii` is `true` (#5344)

Add the check if `sendDefaultPii` is set to `true` before adding the `user_id` when populating DSC.
Additionally, add some tests to check for the changed behaviour.
Conforms with Sentry Dev Spec for handling sensitive data in DSC: https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/#handling-sensitive-data-and-pii

* docs(remix): Add missing comma in README (#5353)

* fix(remix): Sourcemaps upload script is missing in the tarball (#5356)

Add the sourcemaps upload script to the tarball, which required the following changes:
* Add a package-specific `prepack.ts` script which is invoked by the main `prepack.ts` script. This is similar to how we do it with the Gatsby SDK where - just as with Remix - we need to copy over additional files to the `build` directory that are not part of our other packages. The newly added files are:
  * `scripts/sentry-upload-sourcemaps.js`
  * `scripts/createRelease.js`
* Add a package-specific `.npmignore` which extends the root level `.npmignore` file to not ignore the `scripts` directory in the `build` directory.

* docs(remix): Adjust wording in Remix README (#5357)

* meta: Add CHANGELOG for 7.5.0 (#5354)

Co-authored-by: Lukas Stracke <lukas@stracke.co.at>

* release: 7.5.0

* ref(tracing): Remove transaction name  and user_id from DSC (#5363)

This patch temporarily removes the `user_id` and `transaction` (name) fields from the dynamic sampling context, meaning they will no longer propagated with outgoing requests via the baggage Http header or sent to sentry via the `trace` envelope header.

We're taking this temporary measure to ensure that for the moment PII is not sent to third parties.

* meta: Add changelog for 7.5.1 (#5372)

* release: 7.5.1

* fix(remix): Move hook checks inside the wrapper component. (#5371)

`withSentryRouteTracing` is called on `root.tsx` which runs before `entry.client.tsx` (where `Sentry.init` for browser is called and `remixRouterInstrumentation` is assigned). 

The check if the provided hooks are available was failing silently, and causing `withSentryRouteTracing` to early return the `App` unwrapped.

This PR ensures that validation runs on the client side, after `entry.client.tsx`.

* fix(remix): Strip query params from transaction names (#5368)

* feat(tracing): Add transaction source field (#5367)

This patch adds `source` information to `Transaction`, which is typed by
`TransactionSource` in `@sentry/tracing`. This helps track how the name
of a transaction was determined, which will be used by the server for
server-side controls.

For now, we are placing the `source` field under transaction metadata.
In the future, we can move this up into a top level API (an argument to
`startTransaction` or `transaction.setSource`) if needed, but this
should be fine to get us started.

For next steps, after this patch gets merged, we will start going
through various routing instrumentation frameworks and adding
transaction source.

* build: Build with frozen lockfile (#5348)

* fix(remix): Make peer deps less restrictive (#5369)

* ref(build): Reenable lambda layer release in craft (#5207)

* ref(tracing): Add transaction source to default router (#5386)

* ref(react): Add source to react-router-v3 (#5377)

* ref(angular): Add transaction source for Angular Router (#5382)

Add the transaction name source annotation to the Angular SDK. Since the Angular SDK currently does not parameterize URLs, we only assign `'url'` as the transaction name source. We're revisiting parameterization in Angular in a follow up PR.

Additionally, add a two tests to cover this change (might be more relevant once we add parameterization).

* feat(vue): Add transaction source to VueRouter instrumentation (#5381)

* ref(react): Add transaction source for react router v6 (#5385)

* ref(react): Add transaction source for react router v4/v5 (#5384)

* feat(remix): Wrap root with `ErrorBoundary`. (#5365)

Wraps the Remix root with `ErrorBoundary` while wrapping it with the router instrumentation.

* fix(remix): Wrap `handleDocumentRequest` functions. (#5387)

We're currently wrapping `action` and `loader` functions of Remix routes for tracing and error capturing.

When testing the case in #5362, I realized the `render` function of a SSR component in Remix has another handler
[`handleDocumentRequest`](https://github.com/remix-run/remix/blob/b928040061890a6ef0270abdb5b1201638f0dd00/packages/remix-server-runtime/server.ts#L174) which [doesn't re-throw internal errors](https://github.com/remix-run/remix/blob/main/packages/remix-server-runtime/server.ts#L502-L507) so we can't catch them in `wrapRequestHandler`.

Also added a tracing span for `handleDocumentRequest`.

Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>

* ref(tracing): Include transaction in DSC if transaction source is not an unparameterized URL (#5392)

This patch re-introduces the `transaction` field in the Dynamic Sampling Context (DSC). However, its presence is now determined by the [transaction source](https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations) which was introduced in #5367.

As of this we we add the `transaction` field back, if the source indicates that the transaction name is not an unparameterized URL (meaning, the source is set and it is not `url`). 

Additionally, the PR (once again) adjusts our unit and integration tests to reflect this change. Repurposed some DSC<=>`sendDefaultPii` tests that we previously skipped to now cover the transaction<=>transaction source dependence. Did some cleanup of commented out old code and explanations that no longer apply.

Remove he `'unknown'` field from the `TransactionSource` type because it is only used by Relay and SDKs shouldn't set it.

* feat(nextjs): Record transaction name source when creating transactions (#5391)

This adds information about the source of the transaction name to all transactions created by the nextjs SDK.

* ref(serverless): Add transaction source (#5394)

* feat(tracing): Record transaction name source when name set directly (#5396)

This adds transaction name source to transaction metadata in various situations in which the name is set directly: starting a manual transaction, assigning to the `name` property of a transaction, calling a transaction's `setName` method, and changing the name using `beforeNavigate`.

In order to distinguish manually-created transactions from automatically-created ones, all internal uses of `startTransaction` have been changed so that they are happening directly on the hub, rather than through the top-level `Sentry.startTransaction()` wrapper.

* meta: 7.6.0 CHANGELOG (#5397)

* release: 7.6.0

* fix(remix): Add `documentRequest` function name. (#5404)

* build: Upgrade prettier to 2.7.1 (#5395)

* ref(remix): Add transaction source (#5398)

* fix(remix): Skip capturing `ok` responses as errors. (#5405)

[Remix supports throwing responses from `loader` and `action` functions for its internal `CatchBoundary`](https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders).

They are [catched on the caller level](https://github.com/remix-run/remix/blob/7688da5c75190a2e29496c78721456d6e12e3abe/packages/remix-server-runtime/data.ts#L41-L49), but as we wrap the callees, they are registered as exceptions in the SDK. Being http responses, they end up like `{size: 0}`, which is not meaningful.

This PR skips exception capturing for responses that are not `4xx` or `5xx`.

* chore: Use correct function call in migration docs (#5414)

* fix(core): Add `sentry_client` to auth headers (#5413)

This adds `sentry_client` to the auth headers* we send with every envelope request, as described [in the develop docs](https://develop.sentry.dev/sdk/overview/#authentication).

In order to have access to the SDK metadata, the full `ClientOptions` object is now passed to `getEnvelopeEndpointWithUrlEncodedAuth`. Despite the fact that this is really an internal function, it's exported, so in order to keep everything backwards-compatible, for the moment it will also accept a string as the second argument, as it has in the past. Finally, all of our internal uses of the function have been switched to passing `options`, and there's a `TODO` in place so that we remember to remove the backwards compatibility in v8.

Note that this change doesn't affect anyone using a tunnel, as no auth headers are sent in that case, in order to better cloak store requests from ad blockers.

_\*The "headers" are actually querystring values, so as not to trigger CORS issues, but the effect is the same_

Fixes getsentry/sentry-javascript#5406

* feat(angular): Add URL Parameterization of Transaction Names (#5416)

Introduce URL Parameterization to our Angular SDK. With this change, the SDK will update transaction names coming from a URL with a paramterized version of the URL (e.g `GET /users/1235/details` will be parameterized to `GET /users/:userId/details/`).

This is done by listening to the `ResolveEnd` routing event in `TraceService`. When this event is fired, the Angular router has finished resolving the new URL and found the correct route. Besides the url, the event contains a snapshot of the resolved and soon-to-be activated route. This `ActivatedRouteSnapshot` instance is the root instance of the activated route. If the resolved route is something other than `''` or `'/'`, it will also points to its first child. This instance might again point to its (a possibly existing) child but it also holds its part of the resolved and parameterized route path (URL). 
By recursively concatenating the paths, we get a fully parameterized URL. 

The main advantage of this approach vs. a previously tried URL<->parameter interpolation approach is that we do not run into the danger of making interpolation mistakes or leaving parts of the route unparameterized. We now simply take what the Angular router has already resolved.

The potential disadvantage is that we rely on the assumption that there is only one child per route snapshot. While testing, I didn't come across a situation where a parent snapshot had more than one child. I believe that this is because the `ResolveEnd` event contains a snapshot of the newly activated route and not the complete router state. However, if we get reports of incorrect transaction names, we might want to revisit this parameterization approach.  

It should be noted that because there is a gap between transaction creation (where we initially set the unparameterized name) and URL parameterization, it is possible that parameterization might happen after an outgoing Http request is made. In that case, the dynamic sampling context will be populated and frozen without the `transaction` field because at this point the transaction name's source is still `url`. This means that we have a potential timing problem, similar to other routing instrumentations. 
At this point we're not yet sure how often such a timing problem would occur but it seems pretty unlikely for normal usage. For instance, DSC population will happen correctly (with the `transaction` field) when the first outgoing Http request is fired in the constructor of the component that is associated with the new route. This also means that hooks like `ngOnInit` which are frequently used for making requests (e.g. via Service calls) are called long after the `ResolveEnd` routing event.  

Additionally, this add a few unit tests to test this new behaviour. However, these tests are really unit tests, meaning they only test our `TraceService` implementation. We currently simply mock the Angular router and fire the routing events manually. A more "wholesome" approach (e.g. calling `router.navigate` instead of manually firing events) would be better but for this we'd need to inject an actual Angular Router into `TraceService`. This is done best with Angular's `TestBed` feature but it currently doesn't work with our testing setup for the Angular SDK. Changing the setup is out of scope for this PR but we'll revisit this and make the necessary changes to improve the testing setup of our Angular SDK.

* meta: Update changelog for version 7.7.0 (#5421)

* release: 7.7.0

* ref(nextjs): Remove compensation for workaround in `_error.js` (#5378)

For a long time, the `_error.js` page we provide `@sentry/nextjs` users contained a workaround for vercel/next.js#8592. When most of the code in `_error.js` was moved into `captureUnderscoreErrorException`, parts of the workaround came along for the ride. Now that that issue has been fixed, the workaround is no longer necessary.

This removes as many of the workaround's vestiges in `captureUnderscoreErrorException` as possible. (We can't remove them all, because the workaround existed in user code rather than ours, and we can't stop people from continuing to use it. This fixes things so that if they do that, it'll just bail gracefully.)

* fix(remix): Clone erroneous responses not to consume their body streams. (#5429)

Fixes: #5423

Ref: #5405 

We were extracting the bodies of 4xx/5xx responses to capture, but Remix also uses them, we should not consume their `body` streams, as they are only available once in response lifespans. 

This PR updates `extractData` to work on a clone response.

* fix(remix): Do not capture 4xx codes from thrown responses. (#5441)

Fixes: #5425 

Ref: #5429, #5405 

As per review getsentry/sentry-javascript#5429 (comment), we need to define how and when we should capture a 4xx error.

We will only capture thrown 5xx responses until then.

* ref(angular): Set ErrorHandler Exception Mechanism to be unhandled by default(#3844)

Previously, exceptions caught in the Sentry `ErrorHandler` defaulted to the generic mechanism, meaning they were marked as `handled: true` and `mechanism: generic`. This patch adds a custom mechanism for these events: `handled: false` (because they were caught in our ErrorHandler) and `mechanism: angular`.

* ref(utils): Improve uuid generation (#5426)

Since [modern browsers](https://caniuse.com/mdn-api_crypto_randomuuid) support `crypto.randomUUID()` we make use of it when generating UUIDs. This patch does the following:

- Shaves ~160 bytes off the browser bundle
- Modern platforms bail out quickly with `crypto.randomUUID()`
- Less modern browsers (including IE11 and Safari < v15.4) use `crypto.getRandomValues()`
- Node.js 
  - `< v15` uses `Math.random()`
  - `v15 > v16.7 uses` `crypto.getRandomValues()`
  - `>= v16.7 uses` `crypto.randomUUID()`
- Validated that all code paths do in fact return valid uuidv4 ids with hyphens removed!
- Added tests to test all different kinds of random value and uuid creation

* fix(nextjs): Stop using `eval` when checking for `sentry-cli` binary (#5447)

In getsentry/sentry-javascript#4988, we switched to using `eval` in `ensureCLIBinaryExists` (called by our build-time config code in the nextjs SDK), in order to prevent Vercel's dependency-tracing algorithm from thinking the binary was a (n enormous) dependency and including it in people's serverless functions. (It was getting tricked by the `require.resolve()` call; turning that call into a string was the only way we could find to hide it from the algorithm.)

But `eval` is kind of gross. And Rollup, which agrees it's gross, keeps yelling at us for using it. In order to suppress the warnings and generally clean things up, this replaces the `eval` with real code again, and in that real code replaces the `require.resolve()` call with a manual check of all of the paths `require.resolve()` would consider. No `require` means no confused algorithm means no erroneously bundled cli binary in Vercel. And no `eval` means happy Rollup means happy us, because now it's easier to see when the build has legit errors. Wins all around.

* fix: no xhr transport

* fix some type issues

Co-authored-by: Jake Correa <jcorrea257@gmail.com>
Co-authored-by: Lukas Stracke <lukas@stracke.co.at>
Co-authored-by: Niko Felger <niko.felger@gmail.com>
Co-authored-by: Luca Forstner <luca.forstner@sentry.io>
Co-authored-by: getsentry-bot <bot@sentry.io>
Co-authored-by: getsentry-bot <bot@getsentry.com>
Co-authored-by: Onur Temizkan <onur@narval.co.uk>
Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
Co-authored-by: Anton Pirker <anton@ignaz.at>
Co-authored-by: Katie Byers <lobsterkatie@gmail.com>
Co-authored-by: Tim Fish <tim@timfish.uk>
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.

Remix: Throwing a 4XX/5XX Response causes a "body already used for" error
3 participants