Functions logging #2245

sobolk · 2024-11-19T01:13:58Z

Problem

Implements feature request #2058 .

Changes

Add types and converters in platform core for cdk enums
Add new package entry point in platform core to avoid loading cdk eagerly for other purposes (bundling)
Adds cdk as peer dep to platform core (it's already transitive dep via platform-types)
Adds type aliases in backend functions and backend ai packages.

DX

Validation

Added tests

Checklist

If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
If this PR requires a change to the Project Architecture README, I have included that update in this PR.
If this PR requires a docs update, I have linked to that docs PR above.
If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

changeset-bot · 2024-11-19T15:06:19Z

🦋 Changeset detected

Latest commit: 23a7409

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@aws-amplify/ai-constructs	Minor
@aws-amplify/backend-ai	Minor
@aws-amplify/backend-function	Minor
@aws-amplify/backend	Minor
@aws-amplify/platform-core	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ataylorme · 2024-11-27T03:41:09Z

@sobolk Why logging only and not all CDK supported parameters? See the conversation in #1971

I am grateful for this PR, it is definitely and improvement, just curious about the reasoning behind the limited scope

sobolk · 2024-11-27T03:45:06Z

@sobolk Why logging only and not all CDK supported parameters? See the conversation in #1971

I am grateful for this PR, it is definitely and improvement, just curious about the reasoning behind the limited scope

The reason is that we're using some of these parameters for our internal implementation purposes. Therefore, exposing them in uncontrolled way will impact some of our functionalities like secret resolution or bundling.

For "just give me all CDK knobs" we're evaluating different approach described here #1543 and prototyped here #1602 .

ataylorme · 2024-11-27T04:23:41Z

Thank you for the additional context. If someone really wants all the CDK knobs the can use CDK. Some trade offs are expected with a managed solution such as Amplify.

thank you to you and the other contributors for your work on Amplify!

Amplifiyer · 2024-11-27T15:08:33Z

packages/ai-constructs/src/conversation/conversation_handler_construct.ts

-          retention: RetentionDays.INFINITE,
+          retention: this.props.logging?.retention,


is the default still INFINITE? Should we include it regardless to avoid CDK breaking it in future?

is the default still INFINITE?

yes, but we can be explicit about this.

Amplifiyer · 2024-11-27T15:12:33Z

packages/backend-function/src/factory.test.ts

+      assert.ok(
+        Object.keys(lambdas).some((key) => key.startsWith('LogRetention'))
+      );


no way to assert the actual retention period?

I'll double check.

I found something. added below.

Amplifiyer · 2024-11-27T15:25:47Z

packages/platform-core/src/cdk/enum_converters.ts

+ * Converts LogRetention to CDK types.
+ */
+export class LogRetentionConverter {
+  toRetentionDays = (


The converted type is specific to CDK, we should mention it as such.

Suggested change

toRetentionDays = (

toCDKLogsRetentionDays = (

Amplifiyer · 2024-11-27T15:26:04Z

packages/platform-core/src/cdk/enum_converters.ts

+ * Converts LogLevel to CDK types.
+ */
+export class LogLevelConverter {
+  toApplicationLogLevel = (logLevel: LogLevel | undefined) => {


Suggested change

toApplicationLogLevel = (logLevel: LogLevel | undefined) => {

toCDKLambdaApplicationLogLevel = (logLevel: LogLevel | undefined) => {

Amplifiyer · 2024-11-27T15:34:45Z

packages/backend-function/src/factory.test.ts

+      // Enabling log retention adds extra lambda.
+      template.resourceCountIs('AWS::Lambda::Function', 2);
+      const lambdas = template.findResources('AWS::Lambda::Function');


Is it possible to avoid creating this extra lambda by creating the logGroup ourselves https://repost.aws/questions/QUzCgY0gSIQNGHZc6frGcI7g/log-retention-lambda#ANwKiEC22QQ-uV0r5yi0JdBQ?

This has been tried and adopted in conversation handler.
However, it was easier there because it was a new thing.

Here the problem space is constrained in such a way that:

In order to use custom log group (us or customer) we have to pass reference to function. So log group must be created before calling NodejsFunction ctor.

In order to retain log group naming we need function name first. This is not available until NodejsFunction ctor is called, making it circlular dependency problem.

When working on conversation handler I tried to compute function name before creating log group and nodejsfunction but I was not successful at producing function name that would match implicit cdk algorithm. That computation is internalized and I didn't find a way to implement it using available apis from cdk.

Changing function name and/or log group name algorithm at this point would mean destructive changes.

I wanted to avoid destructive changes, so I pursued a solution based on this extra lambda.

I think the proper solution to expose log group knobs would be to add another alternative to LoggingOptions in the future where we {..existing...} | { logGroupKnobs } .

wdyt ?

Sounds good, at least it's only one extra function for all customer defined lambda functions.

Amplifiyer · 2024-11-27T16:31:18Z

packages/platform-core/API.md

@@ -136,13 +136,13 @@ export type LocalConfigurationFileName = 'usage_data_preferences.json';
 // @public
 class LogLevelConverter {
    // (undocumented)
-    toApplicationLogLevel: (logLevel: LogLevel | undefined) => ApplicationLogLevel | undefined;
+    toCDKApplicationLogLevel: (logLevel: LogLevel | undefined) => ApplicationLogLevel | undefined;


I'd still suggest to include lambda in there, as other AWS services may have different log level names or types.

Suggested change

toCDKApplicationLogLevel: (logLevel: LogLevel | undefined) => ApplicationLogLevel | undefined;

toCDKLambdaApplicationLogLevel: (logLevel: LogLevel | undefined) => CDKLambdaApplicationLogLevel | undefined;

I can change the toCDKApplicationLogLevel.

But return type is already import { ApplicationLogLevel } from 'aws-cdk-lib/aws-lambda';
Even if we alias this for our purposes, customer still needs to import from cdk lib.

btw. I didn't notice Lambda in first comment, sorry.

sobolk added 2 commits November 18, 2024 17:11

Functions logging

17056f0

changeset

0cfba52

sobolk added 5 commits November 19, 2024 09:40

more

367ae90

conversation handler

49fe924

conversation handler

20615b0

Merge remote-tracking branch 'origin/main' into functions-logging

3a90b89

this works

63f89d6

josefaidt mentioned this pull request Nov 26, 2024

Feature: allow all optional props for the NodejsFunction CDK construct to be passed via defineFunction in @aws-amplify/backend-function #1971

Open

4 tasks

sobolk added 5 commits November 26, 2024 17:52

more tests

1699eed

more tests

add274e

more tests

05291c7

Merge remote-tracking branch 'origin/main' into functions-logging

b181192

fix build

f735ed8

fix build

f56b52a

sobolk marked this pull request as ready for review November 27, 2024 03:48

sobolk requested review from a team as code owners November 27, 2024 03:48

Amplifiyer reviewed Nov 27, 2024

View reviewed changes

sobolk added 3 commits November 27, 2024 08:07

pr feedback

8bf43ce

pr feedback

0e3659e

api

6d6c3dd

Amplifiyer reviewed Nov 27, 2024

View reviewed changes

Amplifiyer previously approved these changes Nov 27, 2024

View reviewed changes

pr feedback

23a7409

sobolk dismissed Amplifiyer’s stale review via 23a7409 November 27, 2024 16:42

Amplifiyer approved these changes Nov 27, 2024

View reviewed changes

vigy02 approved these changes Nov 27, 2024

View reviewed changes

sobolk merged commit 65abf6a into main Nov 27, 2024
40 checks passed

sobolk deleted the functions-logging branch November 27, 2024 17:44

sobolk mentioned this pull request Jan 3, 2025

feat: add log retention to defineFunction #2055

Closed

4 tasks

		retention: RetentionDays.INFINITE,
		retention: this.props.logging?.retention,

	toApplicationLogLevel = (logLevel: LogLevel \| undefined) => {
	toCDKLambdaApplicationLogLevel = (logLevel: LogLevel \| undefined) => {

	toCDKApplicationLogLevel: (logLevel: LogLevel \| undefined) => ApplicationLogLevel \| undefined;
	toCDKLambdaApplicationLogLevel: (logLevel: LogLevel \| undefined) => CDKLambdaApplicationLogLevel \| undefined;

Functions logging #2245

Functions logging #2245

Uh oh!

Conversation

sobolk commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

DX

Validation

Checklist

Uh oh!

changeset-bot bot commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

ataylorme commented Nov 27, 2024

Uh oh!

sobolk commented Nov 27, 2024

Uh oh!

ataylorme commented Nov 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sobolk commented Nov 19, 2024 •

edited

Loading

changeset-bot bot commented Nov 19, 2024 •

edited

Loading