Skip to content

feat(remix): Refactor to use new performance APIs #10980

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 8, 2024
Merged

feat(remix): Refactor to use new performance APIs #10980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e303613
ref(remix): Refactor remix to avoid `startTransaction`
mydea Mar 8, 2024
a607ea2
replace `startChild`
mydea Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
238 changes: 113 additions & 125 deletions packages/remix/src/utils/instrumentServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,24 @@
/* eslint-disable max-lines */
import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
continueTrace,
getActiveSpan,
getActiveTransaction,
getClient,
getCurrentScope,
getDynamicSamplingContextFromSpan,
getRootSpan,
handleCallbackErrors,
hasTracingEnabled,
setHttpStatus,
spanToJSON,
spanToTraceHeader,
startSpan,
withIsolationScope,
} from '@sentry/core';
import { captureException, getCurrentHub } from '@sentry/node-experimental';
import type { Hub, Transaction, TransactionSource, WrappedFunction } from '@sentry/types';
import { captureException } from '@sentry/node-experimental';
import type { Span, TransactionSource, WrappedFunction } from '@sentry/types';
import {
addExceptionMechanism,
dynamicSamplingContextToSentryBaggageHeader,
Expand All @@ -24,7 +28,6 @@ import {
loadModule,
logger,
objectify,
tracingContextFromHeaders,
} from '@sentry/utils';

import { DEBUG_BUILD } from './debug-build';
Expand Down Expand Up @@ -186,46 +189,50 @@ function makeWrappedDocumentRequestFunction(remixVersion?: number) {
context: EntryContext,
loadContext?: Record<string, unknown>,
): Promise<Response> {
let res: Response;
// eslint-disable-next-line deprecation/deprecation
const activeTransaction = getActiveTransaction();
const activeSpan = getActiveSpan();
const rootSpan = activeSpan && getRootSpan(activeSpan);

try {
// eslint-disable-next-line deprecation/deprecation
const span = activeTransaction?.startChild({
op: 'function.remix.document_request',
origin: 'auto.function.remix',
name: spanToJSON(activeTransaction).description,
const name = rootSpan ? spanToJSON(rootSpan).description : undefined;

return startSpan(
{
// If we don't have a root span, `onlyIfParent` will lead to the span not being created anyhow
// So we don't need to care too much about the fallback name, it's just for typing purposes....
name: name || '<unknown>',
onlyIfParent: true,
attributes: {
method: request.method,
url: request.url,
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.remix',
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.remix.document_request',
},
});

res = await origDocumentRequestFunction.call(
this,
request,
responseStatusCode,
responseHeaders,
context,
loadContext,
);

span?.end();
} catch (err) {
const isRemixV1 = !FUTURE_FLAGS?.v2_errorBoundary && remixVersion !== 2;

// This exists to capture the server-side rendering errors on Remix v1
// On Remix v2, we capture SSR errors at `handleError`
// We also skip primitives here, as we can't dedupe them, and also we don't expect any primitive SSR errors.
if (isRemixV1 && !isPrimitive(err)) {
await captureRemixServerException(err, 'documentRequest', request);
}

throw err;
}

return res;
},
() => {
return handleCallbackErrors(
() => {
return origDocumentRequestFunction.call(
this,
request,
responseStatusCode,
responseHeaders,
context,
loadContext,
);
},
err => {
const isRemixV1 = !FUTURE_FLAGS?.v2_errorBoundary && remixVersion !== 2;

// This exists to capture the server-side rendering errors on Remix v1
// On Remix v2, we capture SSR errors at `handleError`
// We also skip primitives here, as we can't dedupe them, and also we don't expect any primitive SSR errors.
if (isRemixV1 && !isPrimitive(err)) {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
captureRemixServerException(err, 'documentRequest', request);
}
},
);
},
);
};
};
}
Expand All @@ -242,47 +249,32 @@ function makeWrappedDataFunction(
return origFn.call(this, args);
}

let res: Response | AppData;
// eslint-disable-next-line deprecation/deprecation
const activeTransaction = getActiveTransaction();
const currentScope = getCurrentScope();

try {
// eslint-disable-next-line deprecation/deprecation
const span = activeTransaction?.startChild({
return startSpan(
{
op: `function.remix.${name}`,
origin: 'auto.ui.remix',
name: id,
attributes: {
name,
},
});

if (span) {
// Assign data function to hub to be able to see `db` transactions (if any) as children.
// eslint-disable-next-line deprecation/deprecation
currentScope.setSpan(span);
}

res = await origFn.call(this, args);

// eslint-disable-next-line deprecation/deprecation
currentScope.setSpan(activeTransaction);
span?.end();
} catch (err) {
const isRemixV2 = FUTURE_FLAGS?.v2_errorBoundary || remixVersion === 2;

// On Remix v2, we capture all unexpected errors (except the `Route Error Response`s / Thrown Responses) in `handleError` function.
// This is both for consistency and also avoid duplicates such as primitives like `string` or `number` being captured twice.
// Remix v1 does not have a `handleError` function, so we capture all errors here.
if (isRemixV2 ? isResponse(err) : true) {
await captureRemixServerException(err, name, args.request);
}

throw err;
}

return res;
},
() => {
return handleCallbackErrors(
() => origFn.call(this, args),
err => {
const isRemixV2 = FUTURE_FLAGS?.v2_errorBoundary || remixVersion === 2;

// On Remix v2, we capture all unexpected errors (except the `Route Error Response`s / Thrown Responses) in `handleError` function.
// This is both for consistency and also avoid duplicates such as primitives like `string` or `number` being captured twice.
// Remix v1 does not have a `handleError` function, so we capture all errors here.
if (isRemixV2 ? isResponse(err) : true) {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
captureRemixServerException(err, name, args.request);
}
},
);
},
);
};
}

Expand Down Expand Up @@ -382,47 +374,44 @@ export function createRoutes(manifest: ServerRouteManifest, parentId?: string):
}

/**
* Starts a new transaction for the given request to be used by different `RequestHandler` wrappers.
* Starts a new active span for the given request to be used by different `RequestHandler` wrappers.
*/
export function startRequestHandlerTransaction(
hub: Hub,
name: string,
source: TransactionSource,
request: {
headers: {
'sentry-trace': string;
baggage: string;
};
export function startRequestHandlerSpan<T>(
{
name,
source,
sentryTrace,
baggage,
method,
}: {
name: string;
source: TransactionSource;
sentryTrace: string;
baggage: string;
method: string;
},
): Transaction {
// eslint-disable-next-line deprecation/deprecation
const { traceparentData, dynamicSamplingContext, propagationContext } = tracingContextFromHeaders(
request.headers['sentry-trace'],
request.headers.baggage,
);
// eslint-disable-next-line deprecation/deprecation
hub.getScope().setPropagationContext(propagationContext);

// TODO: Refactor this to `startSpan()`
// eslint-disable-next-line deprecation/deprecation
const transaction = hub.startTransaction({
name,
op: 'http.server',
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.remix',
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
method: request.method,
callback: (span: Span) => T,
): T {
return continueTrace(
{
sentryTrace,
baggage,
},
...traceparentData,
metadata: {
dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
() => {
return startSpan(
{
name,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.remix',
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server',
method,
},
},
callback,
);
},
});

// eslint-disable-next-line deprecation/deprecation
hub.getScope().setSpan(transaction);
return transaction;
);
}

/**
Expand All @@ -445,8 +434,6 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui
}

return withIsolationScope(async isolationScope => {
// eslint-disable-next-line deprecation/deprecation
const hub = getCurrentHub();
const options = getClient()?.getOptions();

let normalizedRequest: Record<string, unknown> = request;
Expand All @@ -473,23 +460,24 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui
return origRequestHandler.call(this, request, loadContext);
}

const transaction = startRequestHandlerTransaction(hub, name, source, {
headers: {
'sentry-trace': request.headers.get('sentry-trace') || '',
return startRequestHandlerSpan(
{
name,
source,
sentryTrace: request.headers.get('sentry-trace') || '',
baggage: request.headers.get('baggage') || '',
method: request.method,
},
method: request.method,
});

const res = (await origRequestHandler.call(this, request, loadContext)) as Response;
async span => {
const res = (await origRequestHandler.call(this, request, loadContext)) as Response;

if (isResponse(res)) {
setHttpStatus(transaction, res.status);
}

transaction.end();
if (isResponse(res)) {
setHttpStatus(span, res.status);
}

return res;
return res;
},
);
});
};
}
Expand Down
39 changes: 22 additions & 17 deletions packages/remix/src/utils/serverAdapters/express.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { getClient, getCurrentHub, hasTracingEnabled, setHttpStatus, withIsolationScope } from '@sentry/core';
import { flush } from '@sentry/node-experimental';
import type { Hub, Transaction } from '@sentry/types';
import type { Hub, Span } from '@sentry/types';
import { extractRequestData, fill, isString, logger } from '@sentry/utils';

import { DEBUG_BUILD } from '../debug-build';
import { createRoutes, getTransactionName, instrumentBuild, startRequestHandlerTransaction } from '../instrumentServer';
import { createRoutes, getTransactionName, instrumentBuild, startRequestHandlerSpan } from '../instrumentServer';
import type {
AppLoadContext,
ExpressCreateRequestHandler,
Expand Down Expand Up @@ -89,19 +89,24 @@ function startRequestHandlerTransactionWithRoutes(
next: ExpressNextFunction,
hub: Hub,
url: URL,
): Transaction | undefined {
): unknown {
const [name, source] = getTransactionName(routes, url);
const transaction = startRequestHandlerTransaction(hub, name, source, {
headers: {
'sentry-trace': (req.headers && isString(req.headers['sentry-trace']) && req.headers['sentry-trace']) || '',

return startRequestHandlerSpan(
{
name,
source,
sentryTrace: (req.headers && isString(req.headers['sentry-trace']) && req.headers['sentry-trace']) || '',
baggage: (req.headers && isString(req.headers.baggage) && req.headers.baggage) || '',
method: req.method,
},
span => {
// save a link to the transaction on the response, so that even if there's an error (landing us outside of
// the domain), we can still finish it (albeit possibly missing some scope data)
(res as AugmentedExpressResponse).__sentrySpan = span;
return origRequestHandler.call(this, req, res, next);
},
method: req.method,
});
// save a link to the transaction on the response, so that even if there's an error (landing us outside of
// the domain), we can still finish it (albeit possibly missing some scope data)
(res as AugmentedExpressResponse).__sentryTransaction = transaction;
return origRequestHandler.call(this, req, res, next);
);
}

function wrapGetLoadContext(origGetLoadContext: () => AppLoadContext): GetLoadContextFunction {
Expand Down Expand Up @@ -168,7 +173,7 @@ export function wrapExpressCreateRequestHandler(
}

export type AugmentedExpressResponse = ExpressResponse & {
__sentryTransaction?: Transaction;
__sentrySpan?: Span;
};

type ResponseEndMethod = AugmentedExpressResponse['end'];
Expand Down Expand Up @@ -201,16 +206,16 @@ function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod {
* @param res The outgoing response for this request, on which the transaction is stored
*/
async function finishSentryProcessing(res: AugmentedExpressResponse): Promise<void> {
const { __sentryTransaction: transaction } = res;
const { __sentrySpan: span } = res;

if (transaction) {
setHttpStatus(transaction, res.statusCode);
if (span) {
setHttpStatus(span, res.statusCode);

// Push `transaction.finish` to the next event loop so open spans have a better chance of finishing before the
// transaction closes, and make sure to wait until that's done before flushing events
await new Promise<void>(resolve => {
setImmediate(() => {
transaction.end();
span.end();
resolve();
});
});
Expand Down
Loading