Skip to content

Commit a238fff

Browse files
authored
fix(aws-serverless): Only start root span in Sentry wrapper if Otel didn't wrap handler (#12407)
Fix span collision by checking if the AWS lambda handler was already wrapped by Otel instrumentation and only if not starting our own root span. The rest of our handler is still being used (i.e. the flushing logic, error capturing, etc) regardless of otel wrapping. Also Adjusted E2E tests to: - more closely resemble the AWS environment - enable auto patching of the handler with the Sentry SDK handler - better check for Otel and manual spans Ref #12409
1 parent 2149597 commit a238fff

File tree

13 files changed

+125
-63
lines changed

13 files changed

+125
-63
lines changed

.github/workflows/build.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -993,7 +993,7 @@ jobs:
993993
[
994994
'angular-17',
995995
'angular-18',
996-
'aws-lambda-layer',
996+
'aws-lambda-layer-cjs',
997997
'cloudflare-astro',
998998
'node-express',
999999
'create-react-app',
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
const Sentry = require('@sentry/aws-serverless');
2+
3+
const http = require('http');
4+
5+
async function handle() {
6+
await Sentry.startSpan({ name: 'manual-span', op: 'test' }, async () => {
7+
await new Promise(resolve => {
8+
http.get('http://example.com', res => {
9+
res.on('data', d => {
10+
process.stdout.write(d);
11+
});
12+
13+
res.on('end', () => {
14+
resolve();
15+
});
16+
});
17+
});
18+
});
19+
}
20+
21+
module.exports = { handle };
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
const { handle } = require('./lambda-function');
2+
const event = {};
3+
const context = {
4+
invokedFunctionArn: 'arn:aws:lambda:us-east-1:123453789012:function:my-lambda',
5+
functionName: 'my-lambda',
6+
};
7+
handle(event, context);

dev-packages/e2e-tests/test-applications/aws-lambda-layer/src/run.js renamed to dev-packages/e2e-tests/test-applications/aws-lambda-layer-cjs/src/run.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@ child_process.execSync('node ./src/run-lambda.js', {
44
stdio: 'inherit',
55
env: {
66
...process.env,
7-
LAMBDA_TASK_ROOT: '.',
8-
_HANDLER: 'handle',
7+
// On AWS, LAMBDA_TASK_ROOT is usually /var/task but for testing, we set it to the CWD to correctly apply our handler
8+
LAMBDA_TASK_ROOT: process.cwd(),
9+
_HANDLER: 'src/lambda-function.handle',
910

1011
NODE_OPTIONS: '--require @sentry/aws-serverless/dist/awslambda-auto',
1112
SENTRY_DSN: 'http://public@localhost:3031/1337',

dev-packages/e2e-tests/test-applications/aws-lambda-layer/start-event-proxy.mjs renamed to dev-packages/e2e-tests/test-applications/aws-lambda-layer-cjs/start-event-proxy.mjs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,6 @@ import { startEventProxyServer } from '@sentry-internal/test-utils';
22

33
startEventProxyServer({
44
port: 3031,
5-
proxyServerName: 'aws-serverless-lambda-layer',
5+
proxyServerName: 'aws-serverless-lambda-layer-cjs',
66
forwardToSentry: false,
77
});
Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
import * as child_process from 'child_process';
2+
import { expect, test } from '@playwright/test';
3+
import { waitForTransaction } from '@sentry-internal/test-utils';
4+
5+
test('Lambda layer SDK bundle sends events', async ({ request }) => {
6+
const transactionEventPromise = waitForTransaction('aws-serverless-lambda-layer-cjs', transactionEvent => {
7+
return transactionEvent?.transaction === 'my-lambda';
8+
});
9+
10+
// Waiting for 1s here because attaching the listener for events in `waitForTransaction` is not synchronous
11+
// Since in this test, we don't start a browser via playwright, we don't have the usual delays (page.goto, etc)
12+
// which are usually enough for us to never have noticed this race condition before.
13+
// This is a workaround but probably sufficient as long as we only experience it in this test.
14+
await new Promise<void>(resolve =>
15+
setTimeout(() => {
16+
resolve();
17+
}, 1000),
18+
);
19+
20+
child_process.execSync('pnpm start', {
21+
stdio: 'ignore',
22+
});
23+
24+
const transactionEvent = await transactionEventPromise;
25+
26+
// shows the SDK sent a transaction
27+
expect(transactionEvent.transaction).toEqual('my-lambda'); // name should be the function name
28+
expect(transactionEvent.contexts?.trace).toEqual({
29+
data: {
30+
'sentry.sample_rate': 1,
31+
'sentry.source': 'custom',
32+
'sentry.origin': 'auto.otel.aws-lambda',
33+
'cloud.account.id': '123453789012',
34+
'faas.id': 'arn:aws:lambda:us-east-1:123453789012:function:my-lambda',
35+
'otel.kind': 'SERVER',
36+
},
37+
origin: 'auto.otel.aws-lambda',
38+
span_id: expect.any(String),
39+
status: 'ok',
40+
trace_id: expect.any(String),
41+
});
42+
43+
expect(transactionEvent.spans).toHaveLength(2);
44+
45+
// shows that the Otel Http instrumentation is working
46+
expect(transactionEvent.spans).toContainEqual(
47+
expect.objectContaining({
48+
data: expect.objectContaining({
49+
'sentry.op': 'http.client',
50+
'sentry.origin': 'auto.http.otel.http',
51+
url: 'http://example.com/',
52+
}),
53+
description: 'GET http://example.com/',
54+
op: 'http.client',
55+
}),
56+
);
57+
58+
// shows that the manual span creation is working
59+
expect(transactionEvent.spans).toContainEqual(
60+
expect.objectContaining({
61+
data: expect.objectContaining({
62+
'sentry.op': 'test',
63+
'sentry.origin': 'manual',
64+
}),
65+
description: 'manual-span',
66+
op: 'test',
67+
}),
68+
);
69+
});

dev-packages/e2e-tests/test-applications/aws-lambda-layer/src/lambda-function.js

Lines changed: 0 additions & 19 deletions
This file was deleted.

dev-packages/e2e-tests/test-applications/aws-lambda-layer/src/run-lambda.js

Lines changed: 0 additions & 2 deletions
This file was deleted.

dev-packages/e2e-tests/test-applications/aws-lambda-layer/tests/basic.test.ts

Lines changed: 0 additions & 36 deletions
This file was deleted.

packages/aws-serverless/src/sdk.ts

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -320,15 +320,20 @@ export function wrapHandler<TEvent, TResult>(
320320
throw e;
321321
} finally {
322322
clearTimeout(timeoutWarningTimer);
323-
span?.end();
323+
if (span && span.isRecording()) {
324+
span.end();
325+
}
324326
await flush(options.flushTimeout).catch(e => {
325327
DEBUG_BUILD && logger.error(e);
326328
});
327329
}
328330
return rv;
329331
}
330332

331-
if (options.startTrace) {
333+
// Only start a trace and root span if the handler is not already wrapped by Otel instrumentation
334+
// Otherwise, we create two root spans (one from otel, one from our wrapper).
335+
// If Otel instrumentation didn't work or was filtered by users, we still want to trace the handler.
336+
if (options.startTrace && !isWrappedByOtel(handler)) {
332337
const eventWithHeaders = event as { headers?: { [key: string]: string } };
333338

334339
const sentryTrace =
@@ -361,3 +366,19 @@ export function wrapHandler<TEvent, TResult>(
361366
});
362367
};
363368
}
369+
370+
/**
371+
* Checks if Otel's AWSLambda instrumentation successfully wrapped the handler.
372+
* Check taken from @opentelemetry/core
373+
*/
374+
function isWrappedByOtel(
375+
// eslint-disable-next-line @typescript-eslint/ban-types
376+
handler: Function & { __original?: unknown; __unwrap?: unknown; __wrapped?: boolean },
377+
): boolean {
378+
return (
379+
typeof handler === 'function' &&
380+
typeof handler.__original === 'function' &&
381+
typeof handler.__unwrap === 'function' &&
382+
handler.__wrapped === true
383+
);
384+
}

0 commit comments

Comments
 (0)