-
-
Notifications
You must be signed in to change notification settings - Fork 365
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
Added Lambda and API Gateway request IDs to Cloudwatch logs when there's a PHP-FPM issue #865
Conversation
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.
Looks good to me, added some comments
Co-authored-by: Matthieu Napoli <[email protected]>
@@ -108,16 +113,19 @@ public function __destruct() | |||
*/ | |||
public function handleRequest(HttpRequestEvent $event, Context $context): HttpResponse | |||
{ | |||
$this->lambdaRequestId = $context->getAwsRequestId(); |
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.
Why are we mutating the handler state?
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.
Yes, this is exactly where I stumbled when working on this locally.
I don't really want to store state in the class, but then that means passing it everywhere.
This led me to realize that in most cases there is no context associated to the log lines, since we boot PHP-FPM before any request comes in.
In the end, I'm not 100% sure about the approach, and #895 may be able to remove the source issue.
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.
Yeh, I agree #895 or something like it, is really the best option here.
No description provided.