-
Notifications
You must be signed in to change notification settings - Fork 97
Update Lambda logging configuration to use non-deprecated AWS CDK properties #2968
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@aws-amplify/backend-function': patch | ||
--- | ||
|
||
Update lambda logging configuration to use non-deprecated AWS CDK properties |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,22 +4,23 @@ import { FunctionLoggingOptions } from './factory.js'; | |
import { | ||
CDKLoggingOptions, | ||
convertLoggingOptionsToCDK, | ||
createLogGroup, | ||
} from './logging_options_parser.js'; | ||
import { ApplicationLogLevel, LoggingFormat } from 'aws-cdk-lib/aws-lambda'; | ||
import { RetentionDays } from 'aws-cdk-lib/aws-logs'; | ||
import { LogGroup } from 'aws-cdk-lib/aws-logs'; | ||
import { App, Stack } from 'aws-cdk-lib'; | ||
|
||
type TestCase = { | ||
type ConversionTestCase = { | ||
input: FunctionLoggingOptions; | ||
expectedOutput: CDKLoggingOptions; | ||
}; | ||
|
||
const testCases: Array<TestCase> = [ | ||
const conversionTestCases: Array<ConversionTestCase> = [ | ||
{ | ||
input: {}, | ||
expectedOutput: { | ||
format: undefined, | ||
level: undefined, | ||
retention: undefined, | ||
}, | ||
}, | ||
{ | ||
|
@@ -29,7 +30,6 @@ const testCases: Array<TestCase> = [ | |
}, | ||
expectedOutput: { | ||
format: LoggingFormat.TEXT, | ||
retention: RetentionDays.THIRTEEN_MONTHS, | ||
level: undefined, | ||
}, | ||
}, | ||
|
@@ -40,17 +40,48 @@ const testCases: Array<TestCase> = [ | |
}, | ||
expectedOutput: { | ||
format: LoggingFormat.JSON, | ||
retention: undefined, | ||
level: ApplicationLogLevel.DEBUG, | ||
}, | ||
}, | ||
]; | ||
|
||
void describe('LoggingOptions converter', () => { | ||
testCases.forEach((testCase, index) => { | ||
conversionTestCases.forEach((testCase, index) => { | ||
void it(`converts to cdk options[${index}]`, () => { | ||
const convertedOptions = convertLoggingOptionsToCDK(testCase.input); | ||
assert.deepStrictEqual(convertedOptions, testCase.expectedOutput); | ||
}); | ||
}); | ||
|
||
void it('createLogGroup creates a log group with proper retention', () => { | ||
const app = new App(); | ||
const stack = new Stack(app, 'TestStack'); | ||
const logGroup = createLogGroup(stack, 'test-function', { | ||
retention: '13 months', | ||
}); | ||
|
||
// Check that the log group was created | ||
assert.ok(logGroup instanceof LogGroup); | ||
|
||
// We can't easily check the CDK properties from the instance | ||
// So we verify the synthesized CloudFormation template | ||
const template = JSON.parse( | ||
JSON.stringify(app.synth().getStackArtifact('TestStack').template), | ||
); | ||
|
||
// Find the LogGroup resource | ||
const logGroupResources = Object.values(template.Resources).filter( | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
(resource: any) => resource.Type === 'AWS::Logs::LogGroup', | ||
); | ||
|
||
// Ensure we found exactly one log group | ||
assert.strictEqual(logGroupResources.length, 1); | ||
|
||
const logGroupResource = logGroupResources[0] as { | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
Properties: { RetentionInDays: number }; | ||
}; | ||
Comment on lines
+81
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I hate using eslint-disable commands. However, the resource names are not following the same naming convention as the amplify-backend repo. Open for other recommendations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should also be able to use aws-cdk-lib/assertions for this with |
||
assert.strictEqual(logGroupResource.Properties.RetentionInDays, 400); // 13 months = 400 days | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,16 +4,46 @@ import { | |
LogLevelConverter, | ||
LogRetentionConverter, | ||
} from '@aws-amplify/platform-core/cdk'; | ||
import { RetentionDays } from 'aws-cdk-lib/aws-logs'; | ||
import { ILogGroup, LogGroup } from 'aws-cdk-lib/aws-logs'; | ||
import { Construct } from 'constructs'; | ||
import { RemovalPolicy } from 'aws-cdk-lib'; | ||
|
||
/** | ||
* CDK Lambda logging options using non-deprecated properties | ||
* - level: ApplicationLogLevel for the lambda function (maps to applicationLogLevelV2) | ||
* - format: Logging format (JSON or TEXT) | ||
* - logGroup: Custom log group for the function (preferred over using logRetention directly) | ||
*/ | ||
export type CDKLoggingOptions = { | ||
level?: ApplicationLogLevel; | ||
retention?: RetentionDays; | ||
format?: LoggingFormat; | ||
logGroup?: ILogGroup; | ||
}; | ||
|
||
/** | ||
* Converts logging options to CDK. | ||
* Creates a LogGroup with the specified retention settings | ||
* This replaces the deprecated logRetention property on NodejsFunction | ||
*/ | ||
export const createLogGroup = ( | ||
scope: Construct, | ||
id: string, | ||
loggingOptions: FunctionLoggingOptions, | ||
): ILogGroup => { | ||
const retentionDays = new LogRetentionConverter().toCDKRetentionDays( | ||
loggingOptions.retention, | ||
); | ||
|
||
return new LogGroup(scope, `${id}-log-group`, { | ||
retention: retentionDays, | ||
removalPolicy: RemovalPolicy.DESTROY, // This matches Lambda's default for auto-created log groups | ||
}); | ||
}; | ||
|
||
/** | ||
* Converts logging options to CDK format using non-deprecated properties | ||
* Note: This function no longer includes 'retention' in the return object | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you remove the note about retention being removed from here and references to the deprecated |
||
* as that property is deprecated. Instead, create a LogGroup with | ||
* createLogGroup() when needed. | ||
*/ | ||
export const convertLoggingOptionsToCDK = ( | ||
loggingOptions: FunctionLoggingOptions, | ||
|
@@ -24,18 +54,18 @@ export const convertLoggingOptionsToCDK = ( | |
loggingOptions.level, | ||
); | ||
} | ||
const retention = new LogRetentionConverter().toCDKRetentionDays( | ||
loggingOptions.retention, | ||
); | ||
|
||
const format = convertFormat(loggingOptions.format); | ||
|
||
return { | ||
level, | ||
retention, | ||
format, | ||
}; | ||
}; | ||
|
||
/** | ||
* Converts format string to LoggingFormat enum | ||
*/ | ||
const convertFormat = (format: 'json' | 'text' | undefined) => { | ||
switch (format) { | ||
case undefined: | ||
|
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'm not too proud of this, however, I can see that
template
is also defined ultimate asany
. If someone has a better idea how to implement this, feel free to let me know.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.
It looks like you might be able to get the same functionality out of
template.resourceCountIs()
(which is a part of theaws-cdk-lib/assertions
library that is already used in this file). See: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.assertions-readme.html#counting-resources -- you were doing this in the factory test file as well