Add tenant id to lambda context and structured log messages #540
Conversation
There was a problem hiding this comment.
Awesome job @ramisa2108 ! 💯
I left only some small comments, let me know if that makes sense
| @@ -333,6 +334,31 @@ public void nextTest() { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| @@ -581,7 +581,8 @@ public ByteArrayOutputStream call(InvocationRequest request) throws Error, Excep | |||
| cognitoIdentity, | |||
| LambdaEnvironment.FUNCTION_VERSION, | |||
| request.getInvokedFunctionArn(), | |||
| clientContext | |||
| clientContext, | |||
There was a problem hiding this comment.
I think it would be great to leave the clientContext as the last parameter? This way we keep aws-related fields grouped. WDYT?
| @@ -34,7 +35,8 @@ public LambdaContext( | |||
| CognitoIdentity identity, | |||
| String functionVersion, | |||
| String invokedFunctionArn, | |||
| ClientContext clientContext | |||
| ClientContext clientContext, | |||
There was a problem hiding this comment.
same here, maybe it would be better to swap args
| mockResponse.setHeader("lambda-runtime-aws-request-id", "1234567890"); | ||
| mockResponse.setHeader("Content-Type", "application/json"); | ||
| String expectedTenantId = "my-tenant-id"; | ||
| mockResponse.setHeader("lambda-runtime-aws-tenant-id", expectedTenantId); |
There was a problem hiding this comment.
I think it would be great to have another set of tests for:
- header not present
- header present and empty string
- header present and null value
There was a problem hiding this comment.
Thanks for the suggestion. Added these test cases.
| @@ -61,6 +61,11 @@ struct invocation_request { | |||
| */ | |||
| std::chrono::time_point<std::chrono::system_clock> deadline; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
hopefully at some point, we won't have a copy of aws-lambda-cpp in here, so let's keep that change here but I think you also need to modify the source of truth here: https://github.com/awslabs/aws-lambda-cpp? (Not blocking for this PR to be merged) I've also created an issue for us to track that change at some point: #541
There was a problem hiding this comment.
Agreed, one day, we could get rid of this. (Not blocking PR from my end either).
| @@ -29,7 +29,8 @@ void testFormattingWithLambdaContext() { | |||
| null, | |||
| null, | |||
| "function-arn", | |||
| null | |||
| null, | |||
| "tenant-id" | |||
There was a problem hiding this comment.
let's also add a test with a null value for tenant-id as it's optional
There was a problem hiding this comment.
Added test with null tenant-id.
ramisa2108
force-pushed
the
ramisa2108/support-multi-tenancy
branch
from
30c667c to
b7c13ac
Compare
Issue #, if available:
Description of changes:
Target (OCI, Managed Runtime, both):
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.