-
Notifications
You must be signed in to change notification settings - Fork 113
Add tracing of request events + mention LOG_LEVEL in README #315
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
Conversation
logger.trace("Request", metadata: ["bytes": .string(String(buffer: bytes))]) | ||
logger.debug("sending invocation to lambda handler") |
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 think we should combine those two. debug is the right level imo.
What happens with payloads that aren't string representable. IIRC I can send any payload to lambda.
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.
Thank you @fabianfett for the feedback.
I think we should combine those two. debug is the right level imo.
I like the idea of separating debug output from tracing. Trace adds more info, potentially long data trace, which is the case here.
But maybe it's a matter of personal preference.
I can see user preferences for having just the debug logs, while some others want to have the trace level for a limited period of time.
Given the potentially sensitive nature of the data contained in the payload (such as JWT tokens or PII), I think it is a good idea to decouple the debug and trace levels in this case.
What happens with payloads that aren't string representable
According to the AWS Lambda service API documentation, the payload
is always a string
https://docs.aws.amazon.com/lambda/latest/dg/API_Invoke.html
Furthermore, the max payload size for synchronous invocations is 6Mb. They wont be huge payloads passed as input to the swift AWS Lambda Runtime.
https://docs.aws.amazon.com/lambda/latest/dg/gettingstarted-limits.html
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.
we can also do something as follow if we want to eliminate the two logging statements
if logger.logLevel <= .trace {
logger.trace("...")
} else {
logger.debug("...")
}
}
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.
Furthermore, the max payload size for synchronous invocations is 6Mb
perhaps we want to trim it?
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.
Mostly some nits 🙂
Co-authored-by: Mahdi Bahrami <[email protected]>
Co-authored-by: Mahdi Bahrami <[email protected]>
Co-authored-by: Mahdi Bahrami <[email protected]>
Co-authored-by: Mahdi Bahrami <[email protected]>
@tomerd wdyt ? |
Thank you @MahdiBM for your suggestions - they are all in. |
@tomerd should we reconsider merging this one too ? |
@sebsto I believe https://github.com/swift-server/swift-aws-lambda-runtime/pull/315/files#r1429195527 needs to be addressed first |
@tomerd done. |
// when log level is trace or lower, print the first Kb of the payload | ||
if logger.logLevel <= .trace, let buffer = bytes.getSlice(at: 0, length: max(bytes.readableBytes, 1024)) { | ||
logger.trace("sending invocation to lambda handler", | ||
metadata: ["1024 first bytes": .string(String(buffer: buffer))]) |
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 suspect String(buffer: buffer)
may fail if buffer is not a string, please make sure it does not crash
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.
According to the AWS Lambda service API documentation, the payload
is always a string
https://docs.aws.amazon.com/lambda/latest/dg/API_Invoke.html
Hello @tomerd Ar ewe good to merge with this last comment ? Or you still prefer code to enforce the content type ?
|
This PR adds a
logger.trace()
inLambdaRuntime
just before calling user's handler.It allows to see in the trace logs a string representation of the event passed by the AWS Service.
Motivation:
When there is no or poor documentation of the event passed to a lambda function, is it super hard to figure out the exact
Codable
struct to implement for theLambdaHandler
.With other programming languages, such as Python or Typescript, developers typically add a
print(event)
orconsole.log(event)
at the start of the lambda handler function to figure out the exact type of data to deal with.When using the Swift AWS Lambda Runtime, it's not possible because the bytes received are encoded to a
struct
before the user's code is called.When there is a mismatch between the received bytes and the
Codable
struct, the user is left with very few information to figure out what was wrong. Theencode()
error messages are cryptic. They don't provide enough information to debug this type of error.Modifications:
I added one line in
LambdaRuntime
to trace a string representation of the request before calling the user's handler.I also modified the README to document the use of the LOG_LEVEL environemnt variable.
While I was modifying the README, I noticed there was no mention of local test, I therefore added a small section about local testing (which is a subject related to this PR, hence a single PR for the two new sections in the README)
Result:
When invoking the runtime with
LOG_LEVEL=trace
, we receive information such as