-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(aws): Add SENTRY_LAYER_EXTENSION to configure using the lambda layer extension via env variables
#18101
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
base: develop
Are you sure you want to change the base?
Conversation
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Two minor nits, both optional :)
| test('should support various falsy values (0, no, off)', () => { | ||
| const falsyValues = ['0', 'no', 'off', 'n', 'f']; | ||
|
|
||
| falsyValues.forEach(value => { | ||
| mockGetSDKSource.mockReturnValue('aws-lambda-layer'); | ||
| process.env.SENTRY_LAYER_EXTENSION = value; | ||
|
|
||
| init({}); | ||
|
|
||
| expect(mockInitWithoutDefaultIntegrations).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| useLayerExtension: false, | ||
| }), | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| test('should fall back to default behavior when SENTRY_LAYER_EXTENSION is not set', () => { | ||
| mockGetSDKSource.mockReturnValue('aws-lambda-layer'); | ||
| const options: AwsServerlessOptions = {}; | ||
|
|
||
| init(options); | ||
|
|
||
| expect(mockInitWithoutDefaultIntegrations).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| useLayerExtension: true, | ||
| tunnel: 'http://localhost:9000/envelope', | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: no strong opinion so feel free to ignore but: I don't think we necessarily need to test this here, since (hopefully) envToBool already covers this. The test obviously doesn't hurt but wdyt about converting it to it.each if we keep it? This would make it clear which case fails which we don't see with the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I removed the ones that basically just test envToBool but kept some of the other ones. I kept them as is, not via test.each because it seemed harder to follow when I tried that.
… layer extension via env variables
Co-authored-by: Lukas Stracke <[email protected]>
b9bf109 to
f72dc76
Compare
andreiborza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworked the logic a bit to make sure the tunnel is only set when it's feasible (i.e. aws layer + no prior tunnel + no proxy) and cleaned up the warnings around this a bit.
size-limit report 📦
|
Our recommended approach for the aws sdk is to use our Lambda layer. When the Lambda layer is used, it would previously automatically set up the layer extension. This can be disabled by setting
useLayerExtension: falseduring init, but when using the layer with our auto-wrapping as per recommendation, it wasn't possible to opt out of using the extension.This PR adds
SENTRY_LAYER_EXTENSIONthat can be used to opt out of the extension.