Skip to content

ref(utils): Stop setting transaction in requestDataIntegration #14306

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

Merged
merged 6 commits into from
Nov 15, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions docs/migration/draft-v9-migration-guide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!-- For now this is just a place where we can dump migration guide notes for v9 -->

# Deprecations

## `@sentry/utils`

- Deprecated `AddRequestDataToEventOptions.transaction`. This option effectively doesn't do anything anymore, and will
be removed in v9.
- Deprecated `TransactionNamingScheme` type.

## `@sentry/core`

- Deprecated `transactionNamingScheme` option in `requestDataIntegration`.
7 changes: 6 additions & 1 deletion packages/core/src/integrations/requestdata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ export type RequestDataIntegrationOptions = {
};
};

/** Whether to identify transactions by parameterized path, parameterized path with method, or handler name */
/**
* Whether to identify transactions by parameterized path, parameterized path with method, or handler name.
* @deprecated This option does not do anything anymore, and will be removed in v9.
*/
// eslint-disable-next-line deprecation/deprecation
transactionNamingScheme?: TransactionNamingScheme;
};

Expand Down Expand Up @@ -110,6 +114,7 @@ function convertReqDataIntegrationOptsToAddReqDataOpts(
integrationOptions: Required<RequestDataIntegrationOptions>,
): AddRequestDataToEventOptions {
const {
// eslint-disable-next-line deprecation/deprecation
transactionNamingScheme,
include: { ip, user, ...requestOptions },
} = integrationOptions;
Expand Down
19 changes: 1 addition & 18 deletions packages/remix/src/utils/errors.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,5 @@
import type { AppData, DataFunctionArgs, EntryContext, HandleDocumentRequestFunction } from '@remix-run/node';
import {
captureException,
getActiveSpan,
getClient,
getRootSpan,
handleCallbackErrors,
spanToJSON,
} from '@sentry/core';
import { captureException, getClient, handleCallbackErrors } from '@sentry/core';
import type { Span } from '@sentry/types';
import { addExceptionMechanism, isPrimitive, logger, objectify } from '@sentry/utils';
import { DEBUG_BUILD } from './debug-build';
Expand Down Expand Up @@ -61,19 +54,9 @@ export async function captureRemixServerException(
const objectifiedErr = objectify(err);

captureException(isResponse(objectifiedErr) ? await extractResponseError(objectifiedErr) : objectifiedErr, scope => {
const activeSpan = getActiveSpan();
const rootSpan = activeSpan && getRootSpan(activeSpan);
const activeRootSpanName = rootSpan ? spanToJSON(rootSpan).description : undefined;

scope.setSDKProcessingMetadata({
request: {
...normalizedRequest,
// When `route` is not defined, `RequestData` integration uses the full URL
route: activeRootSpanName
? {
path: activeRootSpanName,
}
: undefined,
},
});

Expand Down
3 changes: 0 additions & 3 deletions packages/remix/src/utils/instrumentServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -314,9 +314,6 @@ function wrapRequestHandler(
isolationScope.setSDKProcessingMetadata({
request: {
...normalizedRequest,
route: {
path: name,
},
},
});

Expand Down
30 changes: 6 additions & 24 deletions packages/utils/src/requestdata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import { getClientIPAddress, ipHeaderNames } from './vendor/getIpAddress';
const DEFAULT_INCLUDES = {
ip: false,
request: true,
transaction: true,
user: true,
};
const DEFAULT_REQUEST_INCLUDES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url'];
Expand All @@ -35,6 +34,8 @@ export type AddRequestDataToEventOptions = {
include?: {
ip?: boolean;
request?: boolean | Array<(typeof DEFAULT_REQUEST_INCLUDES)[number]>;
/** @deprecated This option will be removed in v9. It does not do anything anymore, the `transcation` is set in other places. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** @deprecated This option will be removed in v9. It does not do anything anymore, the `transcation` is set in other places. */
/**
* @deprecated This option will be removed with the next major version of the SDK. If you want to disable capturing of the URL, please use the `request` option instead.
*/

maybe a bit more transparent

Copy link
Member Author

Choose a reason for hiding this comment

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

but the request is quite different from this, isn't it? Opting out of the request there will not affect the transaction being set on the transaction event - though to be honest, it probably did not actually affect it even before, because that was set anyhow by other code already 😅

// eslint-disable-next-line deprecation/deprecation
transaction?: boolean | TransactionNamingScheme;
user?: boolean | Array<(typeof DEFAULT_USER_INCLUDES)[number]>;
};
Expand All @@ -52,6 +53,9 @@ export type AddRequestDataToEventOptions = {
};
};

/**
* @deprecated This type will be removed in v9. It is not in use anymore.
*/
export type TransactionNamingScheme = 'path' | 'methodPath' | 'handler';

/**
Expand All @@ -67,6 +71,7 @@ export type TransactionNamingScheme = 'path' | 'methodPath' | 'handler';
* used instead of the request's route)
*
* @returns A tuple of the fully constructed transaction name [0] and its source [1] (can be either 'route' or 'url')
* @deprecated This method will be removed in v9. It is not in use anymore.
*/
export function extractPathForTransaction(
req: PolymorphicRequest,
Expand Down Expand Up @@ -102,23 +107,6 @@ export function extractPathForTransaction(
return [name, source];
}

function extractTransaction(req: PolymorphicRequest, type: boolean | TransactionNamingScheme): string {
switch (type) {
case 'path': {
return extractPathForTransaction(req, { path: true })[0];
}
case 'handler': {
return (req.route && req.route.stack && req.route.stack[0] && req.route.stack[0].name) || '<anonymous>';
}
case 'methodPath':
default: {
// if exist _reconstructedRoute return that path instead of route.path
const customRoute = req._reconstructedRoute ? req._reconstructedRoute : undefined;
return extractPathForTransaction(req, { path: true, method: true, customRoute })[0];
}
}
}

function extractUserData(
user: {
[key: string]: unknown;
Expand Down Expand Up @@ -379,12 +367,6 @@ export function addRequestDataToEvent(
}
}

if (include.transaction && !event.transaction && event.type === 'transaction') {
// TODO do we even need this anymore?
// TODO make this work for nextjs
event.transaction = extractTransaction(req, include.transaction);
}

return event;
}

Expand Down
63 changes: 2 additions & 61 deletions packages/utils/test/requestdata.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -369,67 +369,6 @@ describe('addRequestDataToEvent', () => {
}
});
});

describe('transaction property', () => {
describe('for transaction events', () => {
beforeEach(() => {
mockEvent.type = 'transaction';
});

test('extracts method and full route path by default`', () => {
const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);

expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName');
});

test('extracts method and full path by default when mountpoint is `/`', () => {
mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', '');
mockReq.baseUrl = '';

const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);

// `subpath/` is the full path here, because there's no router mount path
expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName');
});

test('fallback to method and `originalUrl` if route is missing', () => {
delete mockReq.route;

const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);

expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue');
});

test('can extract path only instead if configured', () => {
const optionsWithPathTransaction = {
include: {
transaction: 'path',
},
} as const;

const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithPathTransaction);

expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName');
});

test('can extract handler name instead if configured', () => {
const optionsWithHandlerTransaction = {
include: {
transaction: 'handler',
},
} as const;

const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithHandlerTransaction);

expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler');
});
});
it('transaction is not applied to non-transaction events', () => {
const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);

expect(parsedRequest.transaction).toBeUndefined();
});
});
});

describe('extractRequestData', () => {
Expand Down Expand Up @@ -763,6 +702,7 @@ describe('extractPathForTransaction', () => {
expectedRoute: string,
expectedSource: TransactionSource,
) => {
// eslint-disable-next-line deprecation/deprecation
const [route, source] = extractPathForTransaction(req, options);

expect(route).toEqual(expectedRoute);
Expand All @@ -778,6 +718,7 @@ describe('extractPathForTransaction', () => {
originalUrl: '/api/users/123/details',
} as PolymorphicRequest;

// eslint-disable-next-line deprecation/deprecation
const [route, source] = extractPathForTransaction(req, {
path: true,
method: true,
Expand Down
Loading