Flush lower level logs to console
https://github.com/aws-powertools/powertools-lambda-typescript/discussions/3142#discussioncomment-10821988
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link
Quality Gate passed
Issues
4 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
17.6% Duplication on New Code
No acknowledgement section found. Please make sure you used the template to open a PR and didn't remove the acknowledgment section. Check the template here: https://github.com/aws-powertools/powertools-lambda-typescript/blob/develop/.github/PULL_REQUEST_TEMPLATE.md#acknowledgment
No related issues found. Please ensure there is an open issue related to this change to avoid significant delays or closure.
Just adding a comment here to get notifications about this PR! Whatever is decided here might make sense, we implemented it in Python and dotnet too.
btw, thanks for submitting the PR @zirkelc! Let's wait to hear from @dreamorosi.
Sorry, I actually wanted to open the PR in my forked repo to get a diff to share with @dreamorosi in https://github.com/aws-powertools/powertools-lambda-typescript/discussions/3142#discussioncomment-10892733
Since it's already open, I will leave it open so we can add comments in code.
Hi @zirkelc, thanks you for the PR.
I have a deadline for this year's re:Invent set for Wednesday afternoon. I'm planning on reviewing your PR by Thursday morning at the latest.
Hey @dreamorosi take your time, I just wanted to share it with you to get your opinion
@dreamorosi do you want me to continue working on this or would like to start a RFC first? I think you also mentioned you wait for feedback from the Lambda for something related to this.
As a sidenote: in one of my projects, I implemented this feature manually by creating a “CustomLogger” class that extends the Logger and overrides the logging functions. Since processLogItem and printLog are private, I had to work around this and override each debug/info/warn/error method instead. Would you consider making these protected so we can go the way of subclassing Logger to add custom features like this?
@dreamorosi do you want me to continue working on this or would like to start a RFC first? I think you also mentioned you wait for feedback from the Lambda for something related to this.
Thanks for bringing this up.
Given that there are still some fundamental questions about the implementation I think a small RFC would be better. This is also because we'll want to design the feature in a way that can be adopted by all versions of Powertools for AWS, and not just this one.
Since I expect there will be some moderate discussion and we'll have to make some tests that don't necessarily require an implementation, I think having this discussion in an RFC that we, and other versions, can then use as implementation guide would be best.
If possible, and you're interested in championing this effort, I'd suggest to open a new discussion under RFCs and follow the template[^1], which will inform the subsequent discussion. After that, based on our process, once we reach an agreement we can start the implementation. Just to manage expectations, almost the entire team will be traveling in the US for re:Invent next week, and many (including me) are taking PTO the week afterwards. We'll do our best to engage with the RFC in a timely manner but there will be some delays.
Regarding the discussion with the AWS Lambda team, that is still ongoing but I don't expect to see any meaningful development soon. I think the most pragmatic way forward would be to implement this feature in a way that allows customers to buffer logs and then flush the buffer manually. Then later on, if Lambda decides to make some hook available to improve on this by allowing us to flush when certain conditions happen, we'll add that.
If anything, having a design document / RFC and a clearly defined use case will help the discussion with that team, so I'd say let's move forward without waiting on this.
As a sidenote: in one of my projects, I implemented this feature manually by creating a “CustomLogger” class that extends the
Loggerand overrides the logging functions. SinceprocessLogItemandprintLogare private, I had to work around this and override eachdebug/info/warn/errormethod instead. Would you consider making theseprotectedso we can go the way of subclassingLoggerto add custom features like this?
Yes, we can do this right away.
If you could open an issue, we can try to land this change in the next release.
[^1]: I'm in the process of moving RFCs to Discussions as they were previously in Issues & I expect the PR to get merged tomorrow. You can however already see the template with the different sections in the PR or by trying to open a new issue using that category, if you want to get a sense of what should be in the RFC.
@dreamorosi sounds like a good plan forward! I'd be happy to start the RFC!
Thank you for your support so far and have a good time at re:Invent!
@dreamorosi sounds like a good plan forward! I'd be happy to start the RFC!
Thank you for your support so far and have a good time at re:Invent!
Thank you!
FYI the RFC template in discussions is now live, you should see it if you try to open a new discussion using the RFC category.
For the time being, would it be ok to close this PR? We can always reopen it, in case we want to reprise working on it instead of starting a new branch.
Yes, of course, I wanted to close this PR anyway. I'll start the RFC in the next few days/weeks.