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): Capture thrown fetch responses. #10166

Merged
merged 2 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/remix/src/utils/instrumentServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,12 @@ function isCatchResponse(response: Response): boolean {
async function extractResponseError(response: Response): Promise<unknown> {
const responseData = await extractData(response);

if (typeof responseData === 'string') {
return responseData;
if (typeof responseData === 'string' && responseData.length > 0) {
return new Error(responseData);
}

if (response.statusText) {
return response.statusText;
return new Error(response.statusText);
}

return responseData;
Expand All @@ -92,7 +92,7 @@ export function wrapRemixHandleError(err: unknown, { request }: DataFunctionArgs
// We are skipping thrown responses here as they are handled by
// `captureRemixServerException` at loader / action level
// We don't want to capture them twice.
// This function if only for capturing unhandled server-side exceptions.
// This function is only for capturing unhandled server-side exceptions.
// https://remix.run/docs/en/main/file-conventions/entry.server#thrown-responses
// https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders
if (isResponse(err) || isRouteErrorResponse(err)) {
Expand Down Expand Up @@ -145,7 +145,7 @@ export async function captureRemixServerException(err: unknown, name: string, re
captureException(isResponse(objectifiedErr) ? await extractResponseError(objectifiedErr) : objectifiedErr, scope => {
// eslint-disable-next-line deprecation/deprecation
const transaction = getActiveTransaction();
const activeTransactionName = transaction ? spanToJSON(transaction) : undefined;
const activeTransactionName = transaction ? spanToJSON(transaction).description : undefined;

scope.setSDKProcessingMetadata({
request: {
Expand Down
11 changes: 6 additions & 5 deletions packages/remix/src/utils/web-fetch.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable complexity */
// Based on Remix's implementation of Fetch API
// https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/
// The MIT License (MIT)
Expand Down Expand Up @@ -70,12 +71,12 @@ export const getSearch = (parsedURL: URL): string => {
export const normalizeRemixRequest = (request: RemixRequest): Record<string, any> => {
const { requestInternalsSymbol, bodyInternalsSymbol } = getInternalSymbols(request);

if (!requestInternalsSymbol) {
throw new Error('Could not find request internals symbol');
if (!requestInternalsSymbol && !request.headers) {
throw new Error('Could not find request headers');
}

const { parsedURL } = request[requestInternalsSymbol];
const headers = new Headers(request[requestInternalsSymbol].headers);
const parsedURL = requestInternalsSymbol ? request[requestInternalsSymbol].parsedURL : new URL(request.url);
const headers = requestInternalsSymbol ? new Headers(request[requestInternalsSymbol].headers) : request.headers;
Comment on lines +74 to +79
Copy link

Choose a reason for hiding this comment

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

The standard fetch API should always work (with remix polyfill, with any other polyfill, or no polyfill but node v18+), so I think the code can be cleaner if getInternalSymbols and the associated code logic is deleted.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We had issues with extracting headers in previous Remix versions, and we need to stay backwards compatible. For reference: #6139 (comment)

Choose a reason for hiding this comment

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

@onurtemizkan thanks for the reference. Yes - Remix uses a Proxy but as long as the code sticks to the Standard API interface, it shouldn't cause problems.

I understand that the issues happened before this layer was added, per your commit at 0ee35f0. I do not know the whole code, but from what I see in this layer, request.headers is only used in this function, and the usage seems consistent with the standard API. I wouldn't expect it to introduce any problems if the code is changed to use the standard API in this normalizeRemixRequest function. Is this wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that we had been reading req.headers directly before 0ee35f0. We still provide official support to the remix version and web-fetch version (as it's dependency) that caused #6139.

We don't rely on those versions being aligned to the current Standard API Interface (which was the reason we ended up vendoring code from them). So, IMO it's not worth risking to revive old resolved issues if this patch resolves the non-polyfilled usage. If we decide in the future to drop support for Remix v1 or Node versions < 18 in the Remix SDK, we can work on a broader cleanup, deleting vendored code (which we generally use as the last resort).

Copy link

Choose a reason for hiding this comment

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

The problem is that we had been reading req.headers directly before 0ee35f0.

Yes - what I mean is that while I do not know the code usage before 0ee35f0, I can see how the normalizeRemixRequest function added in 0ee35f0 commit is reading the headers. I do not think changing this function to read from the Standard API should create any problems or break backward compatibility.

We don't rely on those versions being aligned to the current Standard API Interface (which was the reason we ended up vendoring code from them)

If they are polyfilling the standard Fetch interface and are not aligned with it, then that's kinda their issue and they should fix that...

So, IMO it's not worth risking to revive old resolved issues if this patch resolves the non-polyfilled usage.

Hopefully, this is guaranteed by a test 😅.

All good to leave that cleanup for later!


// Fetch step 1.3
if (!headers.has('Accept')) {
Expand All @@ -88,7 +89,7 @@ export const normalizeRemixRequest = (request: RemixRequest): Record<string, any
contentLengthValue = '0';
}

if (request.body !== null) {
if (request.body !== null && request[bodyInternalsSymbol]) {
const totalBytes = request[bodyInternalsSymbol].size;
// Set Content-Length if totalBytes is a number (that is not NaN)
if (typeof totalBytes === 'number' && !Number.isNaN(totalBytes)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from '../../../common/routes/loader-throw-response.$id';
export { default } from '../../../common/routes/loader-throw-response.$id';
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from '../../common/routes/loader-throw-response.$id';
export { default } from '../../common/routes/loader-throw-response.$id';
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { LoaderFunction } from '@remix-run/node';
import { useLoaderData } from '@remix-run/react';

export const loader: LoaderFunction = async ({ params: { id } }) => {
if (id === '-1') {
throw new Response(null, {
status: 500,
statusText: 'Not found',
});
}

return { message: 'hello world' };
};

export default function LoaderThrowResponse() {
const data = useLoaderData();

return (
<div>
<h1>Loader Throw Response</h1>
<span>{data ? data.message : 'No Data'} </span>
</div>
);
}
30 changes: 29 additions & 1 deletion packages/remix/test/integration/test/server/loader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,33 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
});
});

it('reports a thrown error response the loader', async () => {
const env = await RemixTestEnv.init(adapter);
const url = `${env.url}/loader-throw-response/-1`;

const envelopes = await env.getMultipleEnvelopeRequest({ url, count: 1, envelopeType: ['event'] });
const event = envelopes[0][2];

assertSentryEvent(event, {
exception: {
values: [
{
type: 'Error',
value: 'Not found',
stacktrace: expect.any(Object),
mechanism: {
data: {
function: 'loader',
},
handled: false,
type: 'instrument',
},
},
],
},
});
});

it('correctly instruments a parameterized Remix API loader', async () => {
const env = await RemixTestEnv.init(adapter);
const url = `${env.url}/loader-json-response/123123`;
Expand Down Expand Up @@ -250,7 +277,8 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada

const envelopesCount = await env.countEnvelopes({
url,
envelopeType: ['event'],
envelopeType: 'event',
timeout: 3000,
});

expect(envelopesCount).toBe(0);
Expand Down