Repository navigation
Feature request: Add property to logger class to return currently configured keys #3505
Description
Use case
For introspection code may want to see if an addition key has already been added to the logger before appending it since subsequent appends override the previous values.
Note -- it may be proper to add a similar property to the formatter classes.
Solution/User Experience
current_keys = logger.current_keys
Alternative solutions
Could "know" where the keys are stored in the formatter and directly access them -- that would be fragile.
Acknowledgment
- This feature request meets Powertools for AWS Lambda (Python) Tenets
- Should this be considered in other Powertools for AWS Lambda languages? i.e. Java, TypeScript, and .NET
Activity
gwlester commentedon Dec 14, 2023
If ya'll like the idea I'll work up a sample implementation (may take a while due to the end-of-year holidays).
heitorlessa commentedon Dec 14, 2023
great idea @gwlester - two quick thoughts without looking at the docs: 1/ need to think of alternative names, 2/ error handling needed as customers can bring their own log formatters from scratch (this might not be available).
gwlester commentedon Dec 14, 2023
@heitorlessa , good points. Need to let my subconscious think on those for awhile. Don't expect code until sometime in Q1 of 2024.
heitorlessa commentedon Dec 14, 2023
no rush at all ;) We're working on a big surprise to end the year well OR to start the year with a bang :)
Appreciate your long-standing help in raising the bar on DX!
leandrodamascena commentedon Mar 21, 2024
Hi @gwlester! I'm looking into this issue and would like to know if you plan on submitting a PR soon. I think it will be a really good addition to the Logger utility, because as you said, knowing which keys have already been added helps prevent overwrites and other mistakes.
Thanks.
leandrodamascena commentedon Mar 27, 2024
Hey @gwlester and @heitorlessa! I started working on a PR to include this property/method in our next release and I have some questions before moving forward with the PR.
Name
I can't think a better name than
current_keys. Any ideas?Property or method?
Should it be a property or a method? A property can be cached, but I'm not sure if we can cache it since keys can change during runtime. I think this is more a matter of choosing between using
logger.current_keys()orlogger.current_keys. What are your opinions here?@heitorlessa question
In our documentation, we recommend that customers implementing their own Formatter include
append_keysandclear_statemethods. Why not definecurrent_keysas an abstract method so that customers won't inadvertently break the contract?Show keys and values or only keys?
I'm not sure if we should expose keys and values. Customers may inadvertently expose sensitive data if we expose keys and values. On the other hand, I think customers would like to know the values of the keys so they can decide whether to replace them or not. What do you think?
Remove the default keys?
Should we consider not showing default keys like
level,location,timestamp, and others? Do these keys provide any value, or would it be beneficial to not show them?I'd love to hear your opinion on this PR @gwlester @heitorlessa.
Thanks.
gwlester commentedon Mar 28, 2024
@leandrodamascena, here is my take:
Name
I think current_keys is acceptable.
Property or method?
My vote would be for a method, if we go with property it has to be non-caching.
Show keys and values or only keys?
Either:
Remove the default keys?
No, for completeness return them also.
leandrodamascena commentedon Mar 28, 2024
My vote is for a method too.
I believe you suggested creating a new method because when returning keys and values it is a
Dict, and when returning only Keys, it is aList, correct? Why not include a parameter in thecurrent_keysmethod, such aslogger.current_keys(show_keys_only=True), and return empty values? Does this approach make sense to you?Agree
Thanks
gwlester commentedon Mar 28, 2024
Including a parameter on current_keys works for me.
leandrodamascena commentedon Mar 28, 2024
Thanks for your insights @gwlester! @heitorlessa will return from PTO on Tuesday and I look forward to hearing from him for any additional considerations.
For now, there is an open PR (WIP) and feel free to make comments/suggestions for improvement on it.
leandrodamascena commentedon Mar 29, 2024
While writing tests for the new feature, I noticed that introducing
current_keysas an abstract method might cause issues for existing customers who haven't implemented it yet. This raises the question of how to handle errors. @heitorlessa, what are your thoughts on this? I see we haven't applied a similar approach withdelete_keys, for instance. Personally, I believe that if a customer is developing their own Formatter from scratch, they should be responsible for implementing all necessary methods as per our requirements.github-actions commentedon Apr 12, 2024
This issue is now closed. Please be mindful that future comments are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.
github-actions commentedon Apr 18, 2024
This is now released under 2.37.0 version!