Skip to content

Commit 2761f0f

Browse files
Lms24Lms24
committed
fix(astro): Isolate request instrumentation on server side
1 parent 0830866 commit 2761f0f

File tree

2 files changed

+162
-97
lines changed

2 files changed

+162
-97
lines changed

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

Lines changed: 108 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { captureException, configureScope, getCurrentHub, startSpan } from '@sentry/node';
1+
import { captureException, configureScope, getCurrentHub, runWithAsyncContext, startSpan } from '@sentry/node';
22
import type { Hub, Span } from '@sentry/types';
33
import {
44
addNonEnumerableProperty,
@@ -56,107 +56,123 @@ type AstroLocalsWithSentry = Record<string, unknown> & {
5656
__sentry_wrapped__?: boolean;
5757
};
5858

59-
export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseHandler = (
60-
options = { trackClientIp: false, trackHeaders: false },
61-
) => {
59+
export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseHandler = options => {
60+
const handlerOptions = { trackClientIp: false, trackHeaders: false, ...options };
61+
6262
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();
63+
// if there is an active span, we know that this handle call is nested and hence
64+
// we don't create a new domain for it. If we created one, nested server calls would
65+
// create new transactions instead of adding a child span to the currently active span.
66+
if (getCurrentHub().getScope().getSpan()) {
67+
return instrumentRequest(ctx, next, handlerOptions);
6768
}
68-
addNonEnumerableProperty(locals, '__sentry_wrapped__', true);
69+
return runWithAsyncContext(() => {
70+
return instrumentRequest(ctx, next, handlerOptions);
71+
});
72+
};
73+
};
6974

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

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

78-
const allHeaders: Record<string, string> = {};
79-
headers.forEach((value, key) => {
80-
allHeaders[key] = value;
81-
});
90+
const { dynamicSamplingContext, traceparentData, propagationContext } = tracingContextFromHeaders(
91+
headers.get('sentry-trace') || undefined,
92+
headers.get('baggage'),
93+
);
8294

83-
configureScope(scope => {
84-
scope.setPropagationContext(propagationContext);
95+
const allHeaders: Record<string, string> = {};
96+
headers.forEach((value, key) => {
97+
allHeaders[key] = value;
98+
});
8599

86-
if (options.trackClientIp) {
87-
scope.setUser({ ip_address: ctx.clientAddress });
88-
}
89-
});
100+
configureScope(scope => {
101+
scope.setPropagationContext(propagationContext);
90102

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

161177
/**
162178
* 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 & 5 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+
const getCurrentHubSpy = 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(() => {
@@ -174,11 +186,6 @@ describe('sentryMiddleware', () => {
174186
});
175187

176188
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-
182189
const middleware = handleRequest();
183190

184191
const ctx = {
@@ -261,6 +268,48 @@ describe('sentryMiddleware', () => {
261268

262269
expect(html).toBe(originalHtml);
263270
});
271+
272+
describe('async context isolation', () => {
273+
const runWithAsyncContextSpy = vi.spyOn(SentryNode, 'runWithAsyncContext');
274+
afterEach(() => {
275+
vi.clearAllMocks();
276+
runWithAsyncContextSpy.mockRestore();
277+
});
278+
279+
it('starts a new async context if no span is active', async () => {
280+
getSpanMock.mockReturnValueOnce(undefined);
281+
const handler = handleRequest();
282+
const ctx = {};
283+
const next = vi.fn();
284+
285+
try {
286+
// @ts-expect-error, a partial ctx object is fine here
287+
await handler(ctx, next);
288+
} catch {
289+
// this is fine, it's not required to pass in this test
290+
}
291+
292+
expect(runWithAsyncContextSpy).toHaveBeenCalledTimes(1);
293+
});
294+
295+
it("doesn't start a new async context if a span is active", async () => {
296+
// @ts-expect-error, a empty span is fine here
297+
getSpanMock.mockReturnValueOnce({});
298+
299+
const handler = handleRequest();
300+
const ctx = {};
301+
const next = vi.fn();
302+
303+
try {
304+
// @ts-expect-error, a partial ctx object is fine here
305+
await handler(ctx, next);
306+
} catch {
307+
// this is fine, it's not required to pass in this test
308+
}
309+
310+
expect(runWithAsyncContextSpy).not.toHaveBeenCalled();
311+
});
312+
});
264313
});
265314

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

0 commit comments

Comments
 (0)