Skip to content

Commit

Permalink
fix(remix): Resolve Remix Request API compatibility issues. (#6215)
Browse files Browse the repository at this point in the history
Ref: #6139 and #6139 (comment)

This PR is not a complete solution for all issues mentioned in #6139, but it aims to solve the parsing issue, which should fix auto-instrumented usage.

Remix uses its own implementation of Fetch API [1] on both back-end and front-end, which has different structures storing `headers` and `url`. That has caused `RequestData` integration to not work properly on Remix projects.

I first attempted adding support to the core parser, and `PolymorphicRequest`. But it seemed very complex (if possible), because we have to type Proxy objects and Symbols, which our current TypeScript version doesn't fully support.

So, I vendored / modified a couple of unexported utilities from `@remix-run/web-fetch` [2], to convert request objects to a compatible structure that we can consume in `RequestData` integration.

[1] https://github.com/remix-run/web-std-io/tree/main/packages/fetch
[2] https://www.npmjs.com/package/@remix-run/web-fetch
  • Loading branch information
onurtemizkan committed Nov 17, 2022
1 parent fae0682 commit 0ee35f0
Show file tree
Hide file tree
Showing 5 changed files with 169 additions and 7 deletions.
2 changes: 1 addition & 1 deletion packages/remix/src/performance/client.tsx
@@ -1,4 +1,4 @@
import { ErrorBoundaryProps, WINDOW , withErrorBoundary } from '@sentry/react';
import { ErrorBoundaryProps, WINDOW, withErrorBoundary } from '@sentry/react';
import { Transaction, TransactionContext } from '@sentry/types';
import { logger } from '@sentry/utils';
import * as React from 'react';
Expand Down
16 changes: 13 additions & 3 deletions packages/remix/src/utils/instrumentServer.ts
Expand Up @@ -21,12 +21,14 @@ import {
DataFunctionArgs,
HandleDocumentRequestFunction,
ReactRouterDomPkg,
RemixRequest,
RequestHandler,
RouteMatch,
ServerBuild,
ServerRoute,
ServerRouteManifest,
} from './types';
import { normalizeRemixRequest } from './web-fetch';

// Flag to track if the core request handler is instrumented.
export let isRequestHandlerWrapped = false;
Expand Down Expand Up @@ -91,8 +93,10 @@ async function captureRemixServerException(err: Error, name: string, request: Re
return;
}

const normalizedRequest = normalizeRemixRequest(request as unknown as any);

captureException(isResponse(err) ? await extractResponseError(err) : err, scope => {
scope.setSDKProcessingMetadata({ request });
scope.setSDKProcessingMetadata({ request: normalizedRequest });
scope.addEventProcessor(event => {
addExceptionMechanism(event, {
type: 'instrument',
Expand Down Expand Up @@ -252,6 +256,7 @@ function makeWrappedRootLoader(origLoader: DataFunction): DataFunction {
// Note: `redirect` and `catch` responses do not have bodies to extract
if (isResponse(res) && !isRedirectResponse(res) && !isCatchResponse(res)) {
const data = await extractData(res);

if (typeof data === 'object') {
return json(
{ ...data, ...traceAndBaggage },
Expand Down Expand Up @@ -363,15 +368,20 @@ export function getTransactionName(
function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler {
const routes = createRoutes(build.routes);
const pkg = loadModule<ReactRouterDomPkg>('react-router-dom');
return async function (this: unknown, request: Request, loadContext?: unknown): Promise<Response> {

return async function (this: unknown, request: RemixRequest, loadContext?: unknown): Promise<Response> {
const local = domain.create();
return local.bind(async () => {
const hub = getCurrentHub();
const options = hub.getClient()?.getOptions();
const scope = hub.getScope();

const normalizedRequest = normalizeRemixRequest(request);

if (scope) {
scope.setSDKProcessingMetadata({ request });
scope.setSDKProcessingMetadata({
request: normalizedRequest,
});
}

if (!options || !hasTracingEnabled(options)) {
Expand Down
21 changes: 18 additions & 3 deletions packages/remix/src/utils/types.ts
Expand Up @@ -3,11 +3,26 @@
// Types vendored from @remix-run/server-runtime@1.6.0:
// https://github.com/remix-run/remix/blob/f3691d51027b93caa3fd2cdfe146d7b62a6eb8f2/packages/remix-server-runtime/server.ts
import type * as Express from 'express';
import { Agent } from 'https';
import type { ComponentType } from 'react';

export type RemixRequestState = {
method: string;
redirect: RequestRedirect;
headers: Headers;
parsedURL: URL;
signal: AbortSignal | null;
size: number | null;
};

export type RemixRequest = Request &
Record<symbol | string, RemixRequestState> & {
agent: Agent | ((parsedURL: URL) => Agent) | undefined;
};

export type AppLoadContext = any;
export type AppData = any;
export type RequestHandler = (request: Request, loadContext?: AppLoadContext) => Promise<Response>;
export type RequestHandler = (request: RemixRequest, loadContext?: AppLoadContext) => Promise<Response>;
export type CreateRequestHandlerFunction = (this: unknown, build: ServerBuild, ...args: any[]) => RequestHandler;
export type ServerRouteManifest = RouteManifest<Omit<ServerRoute, 'children'>>;
export type Params<Key extends string = string> = {
Expand Down Expand Up @@ -104,7 +119,7 @@ export interface ServerBuild {
}

export interface HandleDocumentRequestFunction {
(request: Request, responseStatusCode: number, responseHeaders: Headers, context: EntryContext):
(request: RemixRequest, responseStatusCode: number, responseHeaders: Headers, context: EntryContext):
| Promise<Response>
| Response;
}
Expand All @@ -119,7 +134,7 @@ interface ServerEntryModule {
}

export interface DataFunctionArgs {
request: Request;
request: RemixRequest;
context: AppLoadContext;
params: Params;
}
Expand Down
122 changes: 122 additions & 0 deletions packages/remix/src/utils/web-fetch.ts
@@ -0,0 +1,122 @@
// Based on Remix's implementation of Fetch API
// https://github.com/remix-run/web-std-io/tree/main/packages/fetch

import { RemixRequest } from './types';

/*
* Symbol extractor utility to be able to access internal fields of Remix requests.
*/
const getInternalSymbols = (
request: Record<string, unknown>,
): {
bodyInternalsSymbol: string;
requestInternalsSymbol: string;
} => {
const symbols = Object.getOwnPropertySymbols(request);
return {
bodyInternalsSymbol: symbols.find(symbol => symbol.toString().includes('Body internals')) as any,
requestInternalsSymbol: symbols.find(symbol => symbol.toString().includes('Request internals')) as any,
};
};

/**
* Vendored from:
* https://github.com/remix-run/web-std-io/blob/f715b354c8c5b8edc550c5442dec5712705e25e7/packages/fetch/src/utils/get-search.js#L5
*/
export const getSearch = (parsedURL: URL): string => {
if (parsedURL.search) {
return parsedURL.search;
}

const lastOffset = parsedURL.href.length - 1;
const hash = parsedURL.hash || (parsedURL.href[lastOffset] === '#' ? '#' : '');
return parsedURL.href[lastOffset - hash.length] === '?' ? '?' : '';
};

/**
* Convert a Request to Node.js http request options.
* The options object to be passed to http.request
* Vendored / modified from:
* https://github.com/remix-run/web-std-io/blob/f715b354c8c5b8edc550c5442dec5712705e25e7/packages/fetch/src/request.js#L259
*/
export const normalizeRemixRequest = (request: RemixRequest): Record<string, any> => {
const { requestInternalsSymbol, bodyInternalsSymbol } = getInternalSymbols(request);

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

const { parsedURL } = request[requestInternalsSymbol];
const headers = new Headers(request[requestInternalsSymbol].headers);

// Fetch step 1.3
if (!headers.has('Accept')) {
headers.set('Accept', '*/*');
}

// HTTP-network-or-cache fetch steps 2.4-2.7
let contentLengthValue = null;
if (request.body === null && /^(post|put)$/i.test(request.method)) {
contentLengthValue = '0';
}

if (request.body !== null) {
const totalBytes = request[bodyInternalsSymbol].size;
// Set Content-Length if totalBytes is a number (that is not NaN)
if (typeof totalBytes === 'number' && !Number.isNaN(totalBytes)) {
contentLengthValue = String(totalBytes);
}
}

if (contentLengthValue) {
headers.set('Content-Length', contentLengthValue);
}

// HTTP-network-or-cache fetch step 2.11
if (!headers.has('User-Agent')) {
headers.set('User-Agent', 'node-fetch');
}

// HTTP-network-or-cache fetch step 2.15
if (request.compress && !headers.has('Accept-Encoding')) {
headers.set('Accept-Encoding', 'gzip,deflate,br');
}

let { agent } = request;

if (typeof agent === 'function') {
agent = agent(parsedURL);
}

if (!headers.has('Connection') && !agent) {
headers.set('Connection', 'close');
}

// HTTP-network fetch step 4.2
// chunked encoding is handled by Node.js
const search = getSearch(parsedURL);

// Manually spread the URL object instead of spread syntax
const requestOptions = {
path: parsedURL.pathname + search,
pathname: parsedURL.pathname,
hostname: parsedURL.hostname,
protocol: parsedURL.protocol,
port: parsedURL.port,
hash: parsedURL.hash,
search: parsedURL.search,
// @ts-ignore - it does not has a query
query: parsedURL.query,
href: parsedURL.href,
method: request.method,
// @ts-ignore - not sure what this supposed to do
headers: headers[Symbol.for('nodejs.util.inspect.custom')](),
insecureHTTPParser: request.insecureHTTPParser,
agent,

// [SENTRY] For compatibility with Sentry SDK RequestData parser, adding `originalUrl` property.
originalUrl: parsedURL.href,
};

return requestOptions;
};
15 changes: 15 additions & 0 deletions packages/remix/test/integration/test/server/action.test.ts
Expand Up @@ -33,6 +33,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
request: {
method: 'POST',
url,
cookies: expect.any(Object),
headers: {
'user-agent': expect.any(String),
host: 'localhost:8000',
},
},
});
});
Expand Down Expand Up @@ -101,6 +106,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
request: {
method: 'POST',
url,
cookies: expect.any(Object),
headers: {
'user-agent': expect.any(String),
host: 'localhost:8000',
},
},
});

Expand All @@ -116,6 +126,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
request: {
method: 'POST',
url,
cookies: expect.any(Object),
headers: {
'user-agent': expect.any(String),
host: 'localhost:8000',
},
},
});
});
Expand Down

0 comments on commit 0ee35f0

Please sign in to comment.