Feature request: Add property to logger class to return currently configured keys about powertools-lambda-python HOT 13 CLOSED

If ya'll like the idea I'll work up a sample implementation (may take a while due to the end-of-year holidays).

from powertools-lambda-python.

Including a parameter on current_keys works for me.

from powertools-lambda-python.

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).

from powertools-lambda-python.

@heitorlessa , good points. Need to let my subconscious think on those for awhile. Don't expect code until sometime in Q1 of 2024.

from powertools-lambda-python.

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!

from powertools-lambda-python.

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.

from powertools-lambda-python.

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() or logger.current_keys. What are your opinions here?

@heitorlessa question

error handling needed as customers can bring their own log formatters from scratch (this might not be available).

In our documentation, we recommend that customers implementing their own Formatter include append_keys and clear_state methods. Why not define current_keys as 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.

from powertools-lambda-python.

@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:

1. keys and values
2. Keys only with another new method get_key_value

Remove the default keys?
No, for completeness return them also.

from powertools-lambda-python.

Property or method? My vote would be for a method, if we go with property it has to be non-caching.

My vote is for a method too.

Show keys and values or only keys? Either:

1. keys and values
2. Keys only with another new method get_key_value

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 a List, correct? Why not include a parameter in the current_keys method, such as logger.current_keys(show_keys_only=True), and return empty values? Does this approach make sense to you?

Remove the default keys? No, for completeness return them also.

Agree

Thanks

from powertools-lambda-python.

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.

from powertools-lambda-python.

@heitorlessa question

error handling needed as customers can bring their own log formatters from scratch (this might not be available).

In our documentation, we recommend that customers implementing their own Formatter include append_keys and clear_state methods. Why not define current_keys as an abstract method so that customers won't inadvertently break the contract?

While writing tests for the new feature, I noticed that introducing current_keys as 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 with delete_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.

from powertools-lambda-python.

⚠️COMMENT VISIBILITY WARNING⚠️

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.

from powertools-lambda-python.

This is now released under 2.37.0 version!

from powertools-lambda-python.