8000 fix(remix): Prevent capturing pending promises as exceptions. (#6129) · yongdamsh/sentry-javascript@fae7b24 · GitHub
[go: up one dir, main page]
Skip to content

Commit fae7b24

Browse files
onurtemizkanonurtemizkan
authored
fix(remix): Prevent capturing pending promises as exceptions. (getsentry#6129)
The reason we have been getting exceptions from Remix SDK captured as, `{}` even if the response is not an empty object was the pending Promises returned from `extractData`. Implemented the async logic and added another abstraction specifically for error responses, to make sure the errors do not end up misnamed. Based on Remix examples of [how to throw a `json(...)`](https://github.com/remix-run/remix/blob/471ab259fba5adbdd70de71ee7bc7e874c1f4db9/docs/api/conventions.md), this implementation favours: 1 - `response` (if `response` is a `string`) 2 - `response.statusText` 3 - The extracted `response` object, which will eventually be handled by [Node SDK's `eventbuilder`](https://github.com/getsentry/sentry-javascript/blob/4aff9f14bee54a65105c2ec65170ecef19c05b33/packages/node/src/eventbuilder.ts#L60-L63) While extracting data from an error response.
1 parent a430af3 commit fae7b24

File tree

3 files changed

+235
-5
lines changed

3 files changed

+235
-5
lines changed

‎packages/remix/src/utils/instrumentServer.ts

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ function isCatchResponse(response: Response): boolean {
5757

5858
// Based on Remix Implementation
5959
// https://github.com/remix-run/remix/blob/7688da5c75190a2e29496c78721456d6e12e3abe/packages/remix-server-runtime/data.ts#L131-L145
60-
function extractData(response: Response): Promise<unknown> {
60+
async function extractData(response: Response): Promise<unknown> {
6161
const contentType = response.headers.get('Content-Type');
6262

6363
// Cloning the response to avoid consuming the original body stream
@@ -70,14 +70,28 @@ function extractData(response: Response): Promise<unknown> {
7070
return responseClone.text();
7171
}
7272

73-
function captureRemixServerException(err: Error, name: string, request: Request): void {
73+
async function extractResponseError(response: Response): Promise<unknown> {
74+
const responseData = await extractData(response);
75+
76+
if (typeof responseData === 'string') {
77+
return responseData;
78+
}
79+
80+
if (response.statusText) {
81+
return response.statusText;
82+
}
83+
84+
return responseData;
85+
}
86+
87+
async function captureRemixServerException(err: Error, name: string, request: Request): Promise<void> {
7488
// Skip capturing if the thrown error is not a 5xx response
7589
// https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders
7690
if (isResponse(err) && err.status < 500) {
7791
return;
7892
}
7993

80-
captureException(isResponse(err) ? extractData(err) : err, scope => {
94+
captureException(isResponse(err) ? await extractResponseError(err) : err, scope => {
8195
scope.setSDKProcessingMetadata({ request });
8296
scope.addEventProcessor(event => {
8397
addExceptionMechanism(event, {
@@ -128,7 +142,7 @@ function makeWrappedDocumentRequestFunction(
128142

129143
span?.finish();
130144
} catch (err) {
131-
captureRemixServerException(err, 'documentRequest', request);
145+
await captureRemixServerException(err, 'documentRequest', request);
132146
throw err;
133147
}
134148

@@ -165,7 +179,7 @@ function makeWrappedDataFunction(origFn: DataFunction, id: string, name: 'action
165179
currentScope.setSpan(activeTransaction);
166180
span?.finish();
167181
} catch (err) {
168-
captureRemixServerException(err, name, args.request);
182+
await captureRemixServerException(err, name, args.request);
169183
throw err;
170184
}
171185

‎packages/remix/test/integration/app/routes/action-json-response/$id.tsx

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,22 @@ export const action: ActionFunction = async ({ params: { id } }) => {
1717
throw redirect('/action-json-response/-1');
1818
}
1919

20+
if (id === '-3') {
21+
throw json({}, { status: 500, statusText: 'Sentry Test Error' });
22+
}
23+
24+
if (id === '-4') {
25+
throw json({ data: 1234 }, { status: 500 });
26+
}
27+
28+
if (id === '-5') {
29+
throw json('Sentry Test Error [string body]', { status: 500 });
30+
}
31+
32+
if (id === '-6') {
33+
throw json({}, { status: 500 });
34+
}
35+
2036
return json({ test: 'test' });
2137
};
2238

‎packages/remix/test/integration/test/server/action.test.ts

Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,4 +185,204 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
185185
},
186186
});
187187
});
188+
189+
it('handles a thrown `json()` error response with `statusText`', async () => {
190+
const env = await RemixTestEnv.init(adapter);
191+
const url = `${env.url}/action-json-response/-3`;
192+
193+
const envelopes = await env.getMultipleEnvelopeRequest({
194+
url,
195+
count: 2,
196+
method: 'post',
197+
envelopeType: ['transaction', 'event'],
198+
});
199+
200+
const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction');
201+
const [event] = envelopes.filter(envelope => envelope[1].type === 'event');
202+
203+
assertSentryTransaction(transaction[2], {
204+
contexts: {
205+
trace: {
206+
op: 'http.server',
207+
status: 'internal_error',
208+
tags: {
209+
method: 'POST',
210+
'http.status_code': '500',
211+
},
212+
},
213+
},
214+
tags: {
215+
transaction: 'routes/action-json-response/$id',
216+
},
217+
});
218+
219+
assertSentryEvent(event[2], {
220+
exception: {
221+
values: [
222+
{
223+
type: 'Error',
224+
value: 'Sentry Test Error',
225+
stacktrace: expect.any(Object),
226+
mechanism: {
227+
data: {
228+
function: 'action',
229+
},
230+
handled: true,
231+
type: 'instrument',
232+
},
233+
},
234+
],
235+
},
236+
});
237+
});
238+
239+
it('handles a thrown `json()` error response without `statusText`', async () => {
240+
const env = await RemixTestEnv.init(adapter);
241+
const url = `${env.url}/action-json-response/-4`;
242+
243+
const envelopes = await env.getMultipleEnvelopeRequest({
244+
url,
245+
count: 2,
246+
method: 'post',
247+
envelopeType: ['transaction', 'event'],
248+
});
249+
250+
const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction');
251+
const [event] = envelopes.filter(envelope => envelope[1].type === 'event');
252+
253+
assertSentryTransaction(transaction[2], {
254+
contexts: {
255+
trace: {
256+
op: 'http.server',
257+
status: 'internal_error',
258+
tags: {
259+
method: 'POST',
260+
'http.status_code': '500',
261+
},
262+
},
263+
},
264+
tags: {
265+
transaction: 'routes/action-json-response/$id',
266+
},
267+
});
268+
269+
assertSentryEvent(event[2], {
270+
exception: {
271+
values: [
272+
{
273+
type: 'Error',
274+
value: 'Non-Error exception captured with keys: data',
275+
stacktrace: expect.any(Object),
276+
mechanism: {
277+
data: {
278+
function: 'action',
279+
},
280+
handled: true,
281+
type: 'instrument',
282+
},
283+
},
284+
],
285+
},
286+
});
287+
});
288+
289+
it('handles a thrown `json()` error response with string body', async () => {
290+
const env = await RemixTestEnv.init(adapter);
291+
const url = `${env.url}/action-json-response/-5`;
292+
293+
const envelopes = await env.getMultipleEnvelopeRequest({
294+
url,
295+
count: 2,
296+
method: 'post',
297+
envelopeType: ['transaction', 'event'],
298+
});
299+
300+
const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction');
301+
const [event] = envelopes.filter(envelope => envelope[1].type === 'event');
302+
303+
assertSentryTransaction(transaction[2], {
304+
contexts: {
305+
trace: {
306+
op: 'http.server',
307+
status: 'internal_error',
308+
tags: {
309+
method: 'POST',
310+
'http.status_code': '500',
311+
},
312+
},
313+
},
314+
tags: {
315+
transaction: 'routes/action-json-response/$id',
316+
},
317+
});
318+
319+
assertSentryEvent(event[2], {
320+
exception: {
321+
values: [
322+
{
323+
type: 'Error',
324+
value: 'Sentry Test Error [string body]',
325+
stacktrace: expect.any(Object),
326+
mechanism: {
327+
data: {
328+
function: 'action',
329+
},
330+
handled: true,
331+
type: 'instrument',
332+
},
333+
},
334+
],
335+
},
336+
});
337+
});
338+
339+
it('handles a thrown `json()` error response with an empty object', async () => {
340+
const env = await RemixTestEnv.init(adapter);
341+
const url = `${env.url}/action-json-response/-6`;
342+
343+
const envelopes = await env.getMultipleEnvelopeRequest({
344+
url,
345+
count: 2,
346+
method: 'post',
347+
envelopeType: ['transaction', 'event'],
348+
});
349+
350+
const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction');
351+
const [event] = envelopes.filter(envelope => envelope[1].type === 'event');
352+
353+
assertSentryTransaction(transaction[2], {
354+
contexts: {
355+
trace: {
356+
op: 'http.server',
357+
status: 'internal_error',
358+
tags: {
359+
method: 'POST',
360+
'http.status_code': '500',
361+
},
362+
},
363+
},
364+
tags: {
365+
transaction: 'routes/action-json-response/$id',
366+
},
367+
});
368+
369+
assertSentryEvent(event[2], {
370+
exception: {
371+
values: [
372+
{
373+
type: 'Error',
374+
value: 'Non-Error exception captured with keys: [object has no keys]',
375+
stacktrace: expect.any(Object),
376+
mechanism: {
377+
data: {
378+
function: 'action',
379+
},
380+
handled: true,
381+
type: 'instrument',
382+
},
383+
},
384+
],
385+
},
386+
});
387+
});
188388
});

0 commit comments

Comments
 (0)
0