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

Sentry.wrapRemixHandleError missing request information #10002

Closed
3 tasks done
clovis1122 opened this issue Jan 1, 2024 · 4 comments · Fixed by #10166
Closed
3 tasks done

Sentry.wrapRemixHandleError missing request information #10002

clovis1122 opened this issue Jan 1, 2024 · 4 comments · Fixed by #10166

Comments

@clovis1122
Copy link

clovis1122 commented Jan 1, 2024

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/remix

SDK Version

7.91.0

Framework Version

2.4.1

Link to Sentry event

https://recruitercom.sentry.io/issues/4787393394/?project=4504806528253952&query=is%3Aunresolved&referrer=issue-stream&statsPeriod=24h&stream_index=0

SDK Setup

No response

Steps to Reproduce

  1. Throw an error inside a loader function

Expected Result

Sentry log the error with request information

Actual Result

Sentry is logging the error without request information.

@clovis1122
Copy link
Author

I spent some time debugging this locally and think it may be related to this code:

const getInternalSymbols = (
request: Record<string, unknown>,
): {
bodyInternalsSymbol: string;
requestInternalsSymbol: string;
} => {
const symbols = Object.getOwnPropertySymbols(request);
return {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
bodyInternalsSymbol: symbols.find(symbol => symbol.toString().includes('Body internals')) as any,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
requestInternalsSymbol: symbols.find(symbol => symbol.toString().includes('Request internals')) as any,
};
};

I do not see any of these internals defined as I debug this, so this function always returns undefined symbols and Remix fails to normalize the request:

image

I wonder why Sentry needs to do this conversion instead of reading the standard Request directly to get the information as needed?

@onurtemizkan
Copy link
Collaborator

Hi @clovis1122, thanks for reporting.

I have tried to reproduce this locally with the latest SDK and Remix 2.4.1 and did not see any problems with the request data getting attached.

I wonder why Sentry needs to do this conversion instead of reading the standard Request directly to get the information as needed?

We are replicating the behaviour of Remix (web-fetch) while extracting information from requests for consistency.

Could you please provide us a reproduction so we can debug it further?

@clovis1122
Copy link
Author

@onurtemizkan I took a second look at this. It seems the code relies on Remix's polyfill for NodeJS's fetch API. The internals like bodyInternalsSymbol and requestInternalsSymbol are available upon installing Remix's polyfill, like so:

import { installGlobals } from "@remix-run/node";
installGlobals();

However, this polyfill should not be needed, starting on NodeJS v18 which includes the Fetch API enabled by default. I do not use it in my app (which runs on Node v20). Sentry should not read Remix internals either. A backward-compatible, future-proof solution is to read from the standard request directly.

export const normalizeRemixRequest = (request: RemixRequest): Record<string, any> => {

This implementation can be modified to read from request using the standard interface. The request's URL is available via request.url. The header are available via request.headers.

@onurtemizkan
Copy link
Collaborator

@clovis1122, I opened a PR which adds support for non-polyfilled fetch request objects. #10166
Which will hopefully resolve this issue.

Thanks again for reporting.

AbhiPrasad pushed a commit that referenced this issue Jan 17, 2024
Fixes: #10160
Potentially fixes:
#10002

This PR adds support for extracting `header` data from thrown non-Remix
responses (fetch responses) inside loaders / actions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants