Skip to content

Commit bfc63cb

Browse files
Lms24Lms24
authored
fix(astro): Isolate request instrumentation middleware (#9709)
Add scope isolation to our sever-side request instrumentation, similarly to the request handler in Sveltekit.
1 parent abbc57f commit bfc63cb

File tree

3 files changed

+167
-109
lines changed

3 files changed

+167
-109
lines changed

‎packages/astro/src/server/middleware.ts

Lines changed: 113 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,11 @@
1-
import { captureException, configureScope, getCurrentHub, startSpan } from '@sentry/node';
1+
import {
2+
captureException,
3+
configureScope,
4+
continueTrace,
5+
getCurrentHub,
6+
runWithAsyncContext,
7+
startSpan,
8+
} from '@sentry/node';
29
import type { Hub, Span } from '@sentry/types';
310
import {
411
addNonEnumerableProperty,
@@ -56,107 +63,125 @@ type AstroLocalsWithSentry = Record<string, unknown> & {
5663
__sentry_wrapped__?: boolean;
5764
};
5865

59-
export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseHandler = (
60-
options = { trackClientIp: false, trackHeaders: false },
61-
) => {
66+
export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseHandler = options => {
67+
const handlerOptions = { trackClientIp: false, trackHeaders: false, ...options };
68+
6269
return async (ctx, next) => {
63-
// Make sure we don't accidentally double wrap (e.g. user added middleware and integration auto added it)
64-
const locals = ctx.locals as AstroLocalsWithSentry;
65-
if (locals && locals.__sentry_wrapped__) {
66-
return next();
70+
// if there is an active span, we know that this handle call is nested and hence
71+
// we don't create a new domain for it. If we created one, nested server calls would
72+
// create new transactions instead of adding a child span to the currently active span.
73+
if (getCurrentHub().getScope().getSpan()) {
74+
return instrumentRequest(ctx, next, handlerOptions);
6775
}
68-
addNonEnumerableProperty(locals, '__sentry_wrapped__', true);
76+
return runWithAsyncContext(() => {
77+
return instrumentRequest(ctx, next, handlerOptions);
78+
});
79+
};
80+
};
6981

70-
const method = ctx.request.method;
71-
const headers = ctx.request.headers;
82+
async function instrumentRequest(
83+
ctx: Parameters<MiddlewareResponseHandler>[0],
84+
next: Parameters<MiddlewareResponseHandler>[1],
85+
options: MiddlewareOptions,
86+
): Promise<Response> {
87+
// Make sure we don't accidentally double wrap (e.g. user added middleware and integration auto added it)
88+
const locals = ctx.locals as AstroLocalsWithSentry;
89+
if (locals && locals.__sentry_wrapped__) {
90+
return next();
91+
}
92+
addNonEnumerableProperty(locals, '__sentry_wrapped__', true);
7293

73-
const { dynamicSamplingContext, traceparentData, propagationContext } = tracingContextFromHeaders(
74-
headers.get('sentry-trace') || undefined,
75-
headers.get('baggage'),
76-
);
94+
const { method, headers } = ctx.request;
7795

78-
const allHeaders: Record<string, string> = {};
96+
const traceCtx = continueTrace({
97+
sentryTrace: headers.get('sentry-trace') || undefined,
98+
baggage: headers.get('baggage'),
99+
});
100+
101+
const allHeaders: Record<string, string> = {};
102+
103+
if (options.trackHeaders) {
79104
headers.forEach((value, key) => {
80105
allHeaders[key] = value;
81106
});
107+
}
82108

109+
if (options.trackClientIp) {
83110
configureScope(scope => {
84-
scope.setPropagationContext(propagationContext);
85-
86-
if (options.trackClientIp) {
87-
scope.setUser({ ip_address: ctx.clientAddress });
88-
}
111+
scope.setUser({ ip_address: ctx.clientAddress });
89112
});
113+
}
90114

91-
try {
92-
// storing res in a variable instead of directly returning is necessary to
93-
// invoke the catch block if next() throws
94-
const res = await startSpan(
95-
{
96-
name: `${method} ${interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params)}`,
97-
op: 'http.server',
98-
origin: 'auto.http.astro',
99-
status: 'ok',
100-
...traceparentData,
101-
metadata: {
102-
source: 'route',
103-
dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
104-
},
105-
data: {
106-
method,
107-
url: stripUrlQueryAndFragment(ctx.url.href),
108-
...(ctx.url.search && { 'http.query': ctx.url.search }),
109-
...(ctx.url.hash && { 'http.fragment': ctx.url.hash }),
110-
...(options.trackHeaders && { headers: allHeaders }),
111-
},
115+
try {
116+
// storing res in a variable instead of directly returning is necessary to
117+
// invoke the catch block if next() throws
118+
const res = await startSpan(
119+
{
120+
...traceCtx,
121+
name: `${method} ${interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params)}`,
122+
op: 'http.server',
123+
origin: 'auto.http.astro',
124+
status: 'ok',
125+
metadata: {
126+
...traceCtx?.metadata,
127+
source: 'route',
112128
},
113-
async span => {
114-
const originalResponse = await next();
115-
116-
if (span && originalResponse.status) {
117-
span.setHttpStatus(originalResponse.status);
118-
}
119-
120-
const hub = getCurrentHub();
121-
const client = hub.getClient();
122-
const contentType = originalResponse.headers.get('content-type');
123-
124-
const isPageloadRequest = contentType && contentType.startsWith('text/html');
125-
if (!isPageloadRequest || !client) {
126-
return originalResponse;
127-
}
128-
129-
// Type case necessary b/c the body's ReadableStream type doesn't include
130-
// the async iterator that is actually available in Node
131-
// We later on use the async iterator to read the body chunks
132-
// see https://github.com/microsoft/TypeScript/issues/39051
133-
const originalBody = originalResponse.body as NodeJS.ReadableStream | null;
134-
if (!originalBody) {
135-
return originalResponse;
136-
}
137-
138-
const newResponseStream = new ReadableStream({
139-
start: async controller => {
140-
for await (const chunk of originalBody) {
141-
const html = typeof chunk === 'string' ? chunk : new TextDecoder().decode(chunk);
142-
const modifiedHtml = addMetaTagToHead(html, hub, span);
143-
controller.enqueue(new TextEncoder().encode(modifiedHtml));
144-
}
145-
controller.close();
146-
},
147-
});
148-
149-
return new Response(newResponseStream, originalResponse);
129+
data: {
130+
method,
131+
url: stripUrlQueryAndFragment(ctx.url.href),
132+
...(ctx.url.search && { 'http.query': ctx.url.search }),
133+
...(ctx.url.hash && { 'http.fragment': ctx.url.hash }),
134+
...(options.trackHeaders && { headers: allHeaders }),
150135
},
151-
);
152-
return res;
153-
} catch (e) {
154-
sendErrorToSentry(e);
155-
throw e;
156-
}
157-
// TODO: flush if serveless (first extract function)
158-
};
159-
};
136+
},
137+
async span => {
138+
const originalResponse = await next();
139+
140+
if (span && originalResponse.status) {
141+
span.setHttpStatus(originalResponse.status);
142+
}
143+
144+
const hub = getCurrentHub();
145+
const client = hub.getClient();
146+
const contentType = originalResponse.headers.get('content-type');
147+
148+
const isPageloadRequest = contentType && contentType.startsWith('text/html');
149+
if (!isPageloadRequest || !client) {
150+
return originalResponse;
151+
}
152+
153+
// Type case necessary b/c the body's ReadableStream type doesn't include
154+
// the async iterator that is actually available in Node
155+
// We later on use the async iterator to read the body chunks
156+
// see https://github.com/microsoft/TypeScript/issues/39051
157+
const originalBody = originalResponse.body as NodeJS.ReadableStream | null;
158+
if (!originalBody) {
159+
return originalResponse;
160+
}
161+
162+
const decoder = new TextDecoder();
163+
164+
const newResponseStream = new ReadableStream({
165+
start: async controller => {
166+
for await (const chunk of originalBody) {
167+
const html = typeof chunk === 'string' ? chunk : decoder.decode(chunk);
168+
const modifiedHtml = addMetaTagToHead(html, hub, span);
169+
controller.enqueue(new TextEncoder().encode(modifiedHtml));
170+
}
171+
controller.close();
172+
},
173+
});
174+
175+
return new Response(newResponseStream, originalResponse);
176+
},
177+
);
178+
return res;
179+
} catch (e) {
180+
sendErrorToSentry(e);
181+
throw e;
182+
}
183+
// TODO: flush if serverless (first extract function)
184+
}
160185

161186
/**
162187
* This function optimistically assumes that the HTML coming in chunks will not be split

‎packages/astro/test/server/middleware.test.ts

Lines changed: 54 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,18 @@ vi.mock('../../src/server/meta', () => ({
1212

1313
describe('sentryMiddleware', () => {
1414
const startSpanSpy = vi.spyOn(SentryNode, 'startSpan');
15+
16+
const getSpanMock = vi.fn(() => {});
17+
// @ts-expect-error only returning a partial hub here
18+
vi.spyOn(SentryNode, 'getCurrentHub').mockImplementation(() => {
19+
return {
20+
getScope: () => ({
21+
getSpan: getSpanMock,
22+
}),
23+
getClient: () => ({}),
24+
};
25+
});
26+
1527
const nextResult = Promise.resolve(new Response(null, { status: 200, headers: new Headers() }));
1628

1729
afterEach(() => {
@@ -86,10 +98,6 @@ describe('sentryMiddleware', () => {
8698
});
8799

88100
it('attaches tracing headers', async () => {
89-
const scope = { setUser: vi.fn(), setPropagationContext: vi.fn() };
90-
// @ts-expect-error, only passing a partial Scope object
91-
const configureScopeSpy = vi.spyOn(SentryNode, 'configureScope').mockImplementation(cb => cb(scope));
92-
93101
const middleware = handleRequest();
94102
const ctx = {
95103
request: {
@@ -108,17 +116,6 @@ describe('sentryMiddleware', () => {
108116
// @ts-expect-error, a partial ctx object is fine here
109117
await middleware(ctx, next);
110118

111-
expect(configureScopeSpy).toHaveBeenCalledTimes(1);
112-
expect(scope.setPropagationContext).toHaveBeenCalledWith({
113-
dsc: {
114-
release: '1.0.0',
115-
},
116-
parentSpanId: '1234567890123456',
117-
sampled: true,
118-
spanId: expect.any(String),
119-
traceId: '12345678901234567890123456789012',
120-
});
121-
122119
expect(startSpanSpy).toHaveBeenCalledWith(
123120
expect.objectContaining({
124121
metadata: {
@@ -174,11 +171,6 @@ describe('sentryMiddleware', () => {
174171
});
175172

176173
it('injects tracing <meta> tags into the HTML of a pageload response', async () => {
177-
vi.spyOn(SentryNode, 'getCurrentHub').mockImplementation(() => ({
178-
// @ts-expect-error this is fine
179-
getClient: () => ({}),
180-
}));
181-
182174
const middleware = handleRequest();
183175

184176
const ctx = {
@@ -261,6 +253,48 @@ describe('sentryMiddleware', () => {
261253

262254
expect(html).toBe(originalHtml);
263255
});
256+
257+
describe('async context isolation', () => {
258+
const runWithAsyncContextSpy = vi.spyOn(SentryNode, 'runWithAsyncContext');
259+
afterEach(() => {
260+
vi.clearAllMocks();
261+
runWithAsyncContextSpy.mockRestore();
262+
});
263+
264+
it('starts a new async context if no span is active', async () => {
265+
getSpanMock.mockReturnValueOnce(undefined);
266+
const handler = handleRequest();
267+
const ctx = {};
268+
const next = vi.fn();
269+
270+
try {
271+
// @ts-expect-error, a partial ctx object is fine here
272+
await handler(ctx, next);
273+
} catch {
274+
// this is fine, it's not required to pass in this test
275+
}
276+
277+
expect(runWithAsyncContextSpy).toHaveBeenCalledTimes(1);
278+
});
279+
280+
it("doesn't start a new async context if a span is active", async () => {
281+
// @ts-expect-error, a empty span is fine here
282+
getSpanMock.mockReturnValueOnce({});
283+
284+
const handler = handleRequest();
285+
const ctx = {};
286+
const next = vi.fn();
287+
288+
try {
289+
// @ts-expect-error, a partial ctx object is fine here
290+
await handler(ctx, next);
291+
} catch {
292+
// this is fine, it's not required to pass in this test
293+
}
294+
295+
expect(runWithAsyncContextSpy).not.toHaveBeenCalled();
296+
});
297+
});
264298
});
265299

266300
describe('interpolateRouteFromUrlAndParams', () => {

‎packages/core/src/tracing/trace.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,6 @@ export function continueTrace<V>(
226226
* These values can be obtained from incoming request headers,
227227
* or in the browser from `<meta name="sentry-trace">` and `<meta name="baggage">` HTML tags.
228228
*
229-
* It also takes an optional `request` option, which if provided will also be added to the scope & transaction metadata.
230229
* The callback receives a transactionContext that may be used for `startTransaction` or `startSpan`.
231230
*/
232231
export function continueTrace<V>(

0 commit comments

Comments
 (0)