Skip to content
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

Stream conversation logs in sandbox #2073

Merged
merged 16 commits into from
Oct 4, 2024

Conversation

sobolk
Copy link
Member

@sobolk sobolk commented Oct 2, 2024

Problem

Conversation handler logs should show up when executing npx ampx sandbox --stream-function-logs.

Changes

This PR makes the following changes to stream and improve logs in conversation handler.

  1. Adds logs with log level in conversation handler runtime componenets.
    1. Conversation handler lambda is configured to emit structured logs. So that it supports log levels, see https://docs.aws.amazon.com/lambda/latest/dg/nodejs-logging.html for reference. I.e. we set log format to json as opposed to text (which doesn't support application log levels).
    2. We add more info and debug logs around incoming event, bedrock invocations, tools invocation, emitting response. Debug logs dump request/response payloads.
  2. We change Sandbox component to support arbitrary log group attached to lambda function.
    1. Previously Sandbox assumed that log group is /aws/lambda/<functionName>. This however won't work for conversation handler (and later if we expose knobs related to logging configuration.
    2. Conversation handler requires custom log group. I attempted to use <functionName> to mimic default convention, but the problem is that log group is input to function ctor, therefore it's impossible to use function's properties to create log group (as this happens first). The only alternative I found was to generate function name ourselves, but that wasn't great either - components used by function ctor internally are not available to us, so if we generate name it would look differently than other functions. Hence I figured that it's better to just make sandbox support arbitrary log group.
    3. The ListTags call in Sandbox is replaced by GetFunction which gives both information that we need - tags and logGroupName. The AmplifyBackendDeployFullAccess already has necessary permissions to call GetFunction.
  3. I moved outputs generation from backend-ai to ai-construct. So that data construct can pass outputStrategy to it in default scenario.

Out of scope

  1. Setting log level for function at runtime. This should be covered separately with support configuring function log settings #2058 . Meanwhile customers can adjust the level on AWS Console after deployment.
  2. This PR exposes ability to pass outputStrategy to ai construct. The necessary plumbing on data side will be pursued separately. The idea was validated by a temporary change (reverted here)

Validation

  1. Added/modified unit tests.
  2. Manual check

Manual check with default info level:
image

Manual check with log level set to debug via AWS Console:
image
image

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.

@sobolk sobolk added the run-e2e Label that will include e2e tests in PR checks workflow label Oct 2, 2024
Copy link

changeset-bot bot commented Oct 2, 2024

🦋 Changeset detected

Latest commit: 88544ca

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-output-schemas Minor
@aws-amplify/backend Patch
@aws-amplify/sandbox Patch

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

@sobolk sobolk marked this pull request as ready for review October 3, 2024 14:59
@sobolk sobolk requested review from a team as code owners October 3, 2024 14:59
@@ -49,6 +51,7 @@ type ConversationHandlerFunctionProps = {
modelId: string;
region?: string;
}>;
outputStorageStrategy?: BackendOutputStorageStrategy<FunctionOutput>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to what data and auth construct do.

/**
* @internal
*/
outputStorageStrategy?: BackendOutputStorageStrategy<AuthOutput>;

Comment on lines +51 to +52
new GetFunctionCommand({
FunctionName: functionName,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already do have permissions to call GetFunction in AmplifyBackendDeployFullAccess

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we went with ListTags earlier was for performance reasons, since it will be called after each sandbox deployment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was ListTags significantly faster than GetFunction ?

Some considerations:

  1. The number of requests to lambda CP doesn't change. We were calling ListTags per each function.
  2. We eliminated extra CloudFormation calls to resolve ARNs.
  3. The alternative would be to call GetFunctionConfiguration in addition to ListTags. But, making two requests is not great quota and latency wise.
  4. It seems that default quota for GetFunction is much more generous than other CP APIs. See picture below.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synced offline. We'll keep GetFunction given that alternatives are not better.

outputStorageStrategy?.appendToBackendOutputList(functionOutputKey, {
version: '1',
payload: {
definedFunctions: this.resources.lambda.functionName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is not a definedFunction. We should store this under it's own AIConstructOutputKey.

@@ -67,6 +86,7 @@ export class ConversationHandlerFunction
// For custom entry we do bundle SDK as we can't control version customer is coding against.
bundleAwsSDK: !!this.props.entry,
},
loggingFormat: LoggingFormat.JSON,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be able to actually see the debug logs, don't we need to also expose the ApplicationLogLevel property so customers can configure it? if I recall cdk sets the default as INFO?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. I called this out in the "Out of scope" section in the PR description.

packages/sandbox/src/lambda_function_log_streamer.ts Outdated Show resolved Hide resolved
/**
* Expected key that AI conversation output is stored under
*/
export const aiConversationOutputKey = 'AWS::Amplify::AI::Conversation';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the naming here follows ai-constructs/<sub_usecase>
I.e. if we add another AI feature not related to conversation we'd have AWS::Amplify::AI::SomethingNew

@sobolk sobolk merged commit 0a5e51c into main Oct 4, 2024
57 of 58 checks passed
@sobolk sobolk deleted the conversation-handler-logging-improvements branch October 4, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-e2e Label that will include e2e tests in PR checks workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants