Skip to content

ref: Set normalizedRequest instead of request in various places #14305

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 1 commit into from
Nov 20, 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
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,12 @@ test('Sends a transaction for a request to app router', async ({ page }) => {
trace_id: expect.any(String),
});

expect(transactionEvent).toEqual(
expect.objectContaining({
request: {
cookies: {},
headers: expect.any(Object),
url: expect.any(String),
Copy link
Member Author

Choose a reason for hiding this comment

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

this was actually wrong - previously, when we only had headers on the request, we'd incorrectly infer the url from the origin header. However, since we had nothing else to go on, that would be wrong - because a) it would always use http://, and be the path would be missing, so you'd always get e.g. http://localhost:3000 or http://sentry.io. This change actually "fixes" this by simply not having a URL anymore.

},
expect(transactionEvent.request).toEqual({
cookies: {},
headers: expect.objectContaining({
'user-agent': expect.any(String),
}),
);

expect(Object.keys(transactionEvent.request?.headers!).length).toBeGreaterThan(0);
});

// The transaction should not contain any spans with the same name as the transaction
// e.g. "GET /server-component/parameter/[...parameters]"
Expand Down
11 changes: 9 additions & 2 deletions packages/astro/src/server/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ import {
startSpan,
withIsolationScope,
} from '@sentry/node';
import type { Scope, SpanAttributes } from '@sentry/types';
import type { RequestEventData, Scope, SpanAttributes } from '@sentry/types';
import {
addNonEnumerableProperty,
extractQueryParamsFromUrl,
logger,
objectify,
stripUrlQueryAndFragment,
Expand Down Expand Up @@ -111,7 +112,13 @@ async function instrumentRequest(
getCurrentScope().setSDKProcessingMetadata({
// We store the request on the current scope, not isolation scope,
// because we may have multiple requests nested inside each other
request: isDynamicPageRequest ? winterCGRequestToRequestData(request) : { method, url: request.url },
normalizedRequest: (isDynamicPageRequest
? winterCGRequestToRequestData(request)
: {
method,
url: request.url,
query_string: extractQueryParamsFromUrl(request.url),
}) satisfies RequestEventData,
});

if (options.trackClientIp && isDynamicPageRequest) {
Expand Down
4 changes: 2 additions & 2 deletions packages/astro/test/server/middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ describe('sentryMiddleware', () => {
await middleware(ctx, next);

expect(setSDKProcessingMetadataMock).toHaveBeenCalledWith({
request: {
normalizedRequest: {
method: 'GET',
url: '/users',
headers: {
Expand Down Expand Up @@ -254,7 +254,7 @@ describe('sentryMiddleware', () => {
await middleware(ctx, next);

expect(setSDKProcessingMetadataMock).toHaveBeenCalledWith({
request: {
normalizedRequest: {
method: 'GET',
url: '/users',
},
Expand Down
9 changes: 5 additions & 4 deletions packages/bun/src/integrations/bunserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import {
startSpan,
withIsolationScope,
} from '@sentry/core';
import type { IntegrationFn, SpanAttributes } from '@sentry/types';
import { getSanitizedUrlString, parseUrl } from '@sentry/utils';
import type { IntegrationFn, RequestEventData, SpanAttributes } from '@sentry/types';
import { extractQueryParamsFromUrl, getSanitizedUrlString, parseUrl } from '@sentry/utils';

const INTEGRATION_NAME = 'BunServer';

Expand Down Expand Up @@ -76,11 +76,12 @@ function instrumentBunServeOptions(serveOptions: Parameters<typeof Bun.serve>[0]
const url = getSanitizedUrlString(parsedUrl);

isolationScope.setSDKProcessingMetadata({
request: {
normalizedRequest: {
url,
method: request.method,
headers: request.headers.toJSON(),
},
query_string: extractQueryParamsFromUrl(url),
} satisfies RequestEventData,
});

return continueTrace(
Expand Down
2 changes: 1 addition & 1 deletion packages/cloudflare/src/scope-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,5 @@ export function addCultureContext(scope: Scope, cf: IncomingRequestCfProperties)
* Set request data on scope
*/
export function addRequest(scope: Scope, request: Request): void {
scope.setSDKProcessingMetadata({ request: winterCGRequestToRequestData(request) });
scope.setSDKProcessingMetadata({ normalizedRequest: winterCGRequestToRequestData(request) });
}
2 changes: 1 addition & 1 deletion packages/cloudflare/test/request.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ describe('withSentry', () => {
},
);

expect(sentryEvent.sdkProcessingMetadata?.request).toEqual({
expect(sentryEvent.sdkProcessingMetadata?.normalizedRequest).toEqual({
headers: {},
url: 'https://example.com/',
method: 'GET',
Expand Down
8 changes: 5 additions & 3 deletions packages/nextjs/src/common/captureRequestError.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { captureException, withScope } from '@sentry/core';
import type { RequestEventData } from '@sentry/types';
import { headersToDict } from '@sentry/utils';

type RequestInfo = {
path: string;
Expand All @@ -18,10 +20,10 @@ type ErrorContext = {
export function captureRequestError(error: unknown, request: RequestInfo, errorContext: ErrorContext): void {
withScope(scope => {
scope.setSDKProcessingMetadata({
request: {
headers: request.headers,
normalizedRequest: {
headers: headersToDict(request.headers),
method: request.method,
},
} satisfies RequestEventData,
});

scope.setContext('nextjs', {
Expand Down
5 changes: 3 additions & 2 deletions packages/nextjs/src/common/withServerActionInstrumentation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
startSpan,
withIsolationScope,
} from '@sentry/core';
import type { RequestEventData } from '@sentry/types';
import { logger, vercelWaitUntil } from '@sentry/utils';

import { DEBUG_BUILD } from './debug-build';
Expand Down Expand Up @@ -89,9 +90,9 @@ async function withServerActionInstrumentationImplementation<A extends (...args:

isolationScope.setTransactionName(`serverAction/${serverActionName}`);
isolationScope.setSDKProcessingMetadata({
request: {
normalizedRequest: {
headers: fullHeadersObject,
},
} satisfies RequestEventData,
});

return continueTrace(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
withIsolationScope,
withScope,
} from '@sentry/core';
import type { WebFetchHeaders } from '@sentry/types';
import type { RequestEventData, WebFetchHeaders } from '@sentry/types';
import { propagationContextFromHeaders, uuid4, winterCGHeadersToDict } from '@sentry/utils';

import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core';
Expand Down Expand Up @@ -68,9 +68,9 @@ export function wrapGenerationFunctionWithSentry<F extends (...args: any[]) => a
scope.setTransactionName(`${componentType}.${generationFunctionIdentifier} (${componentRoute})`);

isolationScope.setSDKProcessingMetadata({
request: {
normalizedRequest: {
headers: headersDict,
},
} satisfies RequestEventData,
});

const activeSpan = getActiveSpan();
Expand Down
2 changes: 1 addition & 1 deletion packages/nextjs/src/common/wrapMiddlewareWithSentry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export function wrapMiddlewareWithSentry<H extends EdgeRouteHandler>(

if (req instanceof Request) {
isolationScope.setSDKProcessingMetadata({
request: winterCGRequestToRequestData(req),
normalizedRequest: winterCGRequestToRequestData(req),
});
spanName = `middleware ${req.method} ${new URL(req.url).pathname}`;
spanSource = 'url';
Expand Down
6 changes: 3 additions & 3 deletions packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
withIsolationScope,
withScope,
} from '@sentry/core';

import type { RequestEventData } from '@sentry/types';
import type { RouteHandlerContext } from './types';

import { propagationContextFromHeaders, winterCGHeadersToDict } from '@sentry/utils';
Expand Down Expand Up @@ -64,10 +64,10 @@ export function wrapRouteHandlerWithSentry<F extends (...args: any[]) => any>(
);
scope.setPropagationContext(incomingPropagationContext);
scope.setSDKProcessingMetadata({
request: {
normalizedRequest: {
method,
headers: completeHeadersDict,
},
} satisfies RequestEventData,
});
}

Expand Down
5 changes: 3 additions & 2 deletions packages/nextjs/src/common/wrapServerComponentWithSentry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
withIsolationScope,
withScope,
} from '@sentry/core';
import type { RequestEventData } from '@sentry/types';
import { propagationContextFromHeaders, uuid4, vercelWaitUntil, winterCGHeadersToDict } from '@sentry/utils';

import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core';
Expand Down Expand Up @@ -49,9 +50,9 @@ export function wrapServerComponentWithSentry<F extends (...args: any[]) => any>
const headersDict = context.headers ? winterCGHeadersToDict(context.headers) : undefined;

isolationScope.setSDKProcessingMetadata({
request: {
normalizedRequest: {
headers: headersDict,
},
} satisfies RequestEventData,
});

return withIsolationScope(isolationScope, () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export function wrapApiHandlerWithSentry<H extends EdgeRouteHandler>(

if (req instanceof Request) {
isolationScope.setSDKProcessingMetadata({
request: winterCGRequestToRequestData(req),
normalizedRequest: winterCGRequestToRequestData(req),
});
currentScope.setTransactionName(`${req.method} ${parameterizedRoute}`);
} else {
Expand Down
37 changes: 3 additions & 34 deletions packages/node/src/integrations/http/SentryHttpInstrumentation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ import { getRequestInfo } from '@opentelemetry/instrumentation-http';
import { addBreadcrumb, getClient, getIsolationScope, withIsolationScope } from '@sentry/core';
import type { PolymorphicRequest, RequestEventData, SanitizedRequestData, Scope } from '@sentry/types';
import {
extractQueryParamsFromUrl,
getBreadcrumbLogLevelFromHttpStatusCode,
getSanitizedUrlString,
headersToDict,
logger,
parseUrl,
stripUrlQueryAndFragment,
Expand Down Expand Up @@ -145,7 +147,7 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
const normalizedRequest: RequestEventData = {
url: absoluteUrl,
method: request.method,
query_string: extractQueryParams(request),
query_string: extractQueryParamsFromUrl(request.url || ''),
headers: headersToDict(request.headers),
cookies,
};
Expand Down Expand Up @@ -443,36 +445,3 @@ function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope):
// ignore errors if we can't patch stuff
}
}

function extractQueryParams(req: IncomingMessage): string | undefined {
// req.url is path and query string
if (!req.url) {
return;
}

try {
// The `URL` constructor can't handle internal URLs of the form `/some/path/here`, so stick a dummy protocol and
// hostname as the base. Since the point here is just to grab the query string, it doesn't matter what we use.
const queryParams = new URL(req.url, 'http://dogs.are.great').search.slice(1);
return queryParams.length ? queryParams : undefined;
} catch {
return undefined;
}
}

function headersToDict(reqHeaders: Record<string, string | string[] | undefined>): Record<string, string> {
const headers: Record<string, string> = Object.create(null);

try {
Object.entries(reqHeaders).forEach(([key, value]) => {
if (typeof value === 'string') {
headers[key] = value;
}
});
} catch (e) {
DEBUG_BUILD &&
logger.warn('Sentry failed extracting headers from a request object. If you see this, please file an issue.');
}

return headers;
}
8 changes: 6 additions & 2 deletions packages/sveltekit/src/server/handle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,9 @@ export function sentryHandle(handlerOptions?: SentryHandleOptions): Handle {
return withIsolationScope(isolationScope => {
// We only call continueTrace in the initial top level request to avoid
// creating a new root span for the sub request.
isolationScope.setSDKProcessingMetadata({ request: winterCGRequestToRequestData(input.event.request.clone()) });
isolationScope.setSDKProcessingMetadata({
normalizedRequest: winterCGRequestToRequestData(input.event.request.clone()),
});
return continueTrace(getTracePropagationData(input.event), () => instrumentHandle(input, options));
});
};
Expand Down Expand Up @@ -167,7 +169,9 @@ async function instrumentHandle(
name: routeName,
},
async (span?: Span) => {
getCurrentScope().setSDKProcessingMetadata({ request: winterCGRequestToRequestData(event.request.clone()) });
getCurrentScope().setSDKProcessingMetadata({
normalizedRequest: winterCGRequestToRequestData(event.request.clone()),
});
const res = await resolve(event, {
transformPageChunk: addSentryCodeToPage(options),
});
Expand Down
2 changes: 2 additions & 0 deletions packages/utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ export {
extractRequestData,
winterCGHeadersToDict,
winterCGRequestToRequestData,
extractQueryParamsFromUrl,
headersToDict,
} from './requestdata';
export type {
AddRequestDataToEventOptions,
Expand Down
42 changes: 41 additions & 1 deletion packages/utils/src/requestdata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -416,18 +416,58 @@ export function winterCGHeadersToDict(winterCGHeaders: WebFetchHeaders): Record<
return headers;
}

/**
* Convert common request headers to a simple dictionary.
*/
export function headersToDict(reqHeaders: Record<string, string | string[] | undefined>): Record<string, string> {
const headers: Record<string, string> = Object.create(null);

try {
Object.entries(reqHeaders).forEach(([key, value]) => {
if (typeof value === 'string') {
headers[key] = value;
}
});
} catch (e) {
DEBUG_BUILD &&
logger.warn('Sentry failed extracting headers from a request object. If you see this, please file an issue.');
}

return headers;
}

/**
* Converts a `Request` object that implements the `Web Fetch API` (https://developer.mozilla.org/en-US/docs/Web/API/Headers) into the format that the `RequestData` integration understands.
*/
export function winterCGRequestToRequestData(req: WebFetchRequest): PolymorphicRequest {
export function winterCGRequestToRequestData(req: WebFetchRequest): RequestEventData {
const headers = winterCGHeadersToDict(req.headers);

return {
method: req.method,
url: req.url,
query_string: extractQueryParamsFromUrl(req.url),
headers,
// TODO: Can we extract body data from the request?
};
}

/** Extract the query params from an URL. */
export function extractQueryParamsFromUrl(url: string): string | undefined {
// url is path and query string
if (!url) {
return;
}

try {
// The `URL` constructor can't handle internal URLs of the form `/some/path/here`, so stick a dummy protocol and
// hostname as the base. Since the point here is just to grab the query string, it doesn't matter what we use.
const queryParams = new URL(url, 'http://dogs.are.great').search.slice(1);
return queryParams.length ? queryParams : undefined;
} catch {
return undefined;
}
}

function extractNormalizedRequestData(
normalizedRequest: RequestEventData,
{ include }: { include: string[] },
Expand Down
Loading