Skip to content
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

Feature request: logger apppend_key context manager #5864

Closed
1 of 2 tasks
jusdino opened this issue Jan 13, 2025 · 8 comments · Fixed by #5883
Closed
1 of 2 tasks

Feature request: logger apppend_key context manager #5864

jusdino opened this issue Jan 13, 2025 · 8 comments · Fixed by #5883
Assignees
Labels
feature-request feature request need-more-information Pending information to continue

Comments

@jusdino
Copy link

jusdino commented Jan 13, 2025

Use case

There are a lot of cases where it could be useful to log extra context-specific keys such as identifiers in a way that is protected from contaminating across other execution contexts. Uses could include looping over a number of jobs, or even enhancing some branch-specific logic with an key that doesn't make sense when the branch didn't execute. In those cases, it would be really handy to be able to concisely and safely manage log keys in a context manager style.

Solution/User Experience

We could enhance the logger interface to include a standard python context manager so use could look something like:

from aws_lambda_powertools.logging import Logger


logger = Logger()

def event_handler(event, context):
    # ... stuff before processing jobs
    for some_job in a_bunch_of_jobs:
        with logger.append_keys(some_id=some_job['id']):
            do_processing(some_job)

    logger.info("Log doesn't contain some_id")
    # ... non-job-specific stuff


def do_processing(some_job):
    logger.info('Log does contain some_id')
    # ... job-specific stuff

I'm not sure modifying the actual append_keys method makes sense as an interface, but some method/attribute/whatever on the Logger based on your judgement would probably serve equally as well.

Alternative solutions

Acknowledgment

@jusdino jusdino added feature-request feature request triage Pending triage from maintainers labels Jan 13, 2025
Copy link

boring-cyborg bot commented Jan 13, 2025

Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

@anafalcao
Copy link
Collaborator

Hello @jusdino ! Thank you so much for opening this feature request. If I understood it correctly, this can already be achieved using the extra parameter. Check here:
https://docs.powertools.aws.dev/lambda/python/latest/core/logger/#extra-parameter

Arguments passed via the extra parameter are only included in the current log message. They are not retained for future log calls.
Let me know if it solves your question!

@anafalcao anafalcao self-assigned this Jan 15, 2025
@anafalcao anafalcao added the need-more-information Pending information to continue label Jan 15, 2025
@jusdino
Copy link
Author

jusdino commented Jan 15, 2025

Hi @anafalcao! Sorry for being unclear. My intent was to describe a feature that applies across multiple log messages, not just one. It could work similarly to Logger.append_keys() but would clean up the appended keys upon leaving the with block context. This is why I suggested a context manager approach - all log messages emitted, for example, while in a part of a workflow that is specific to a particular identifier could have the identifier appended to the log messages with a single with logger.something(process_id=process_id): statement. Using a context manager becomes much safer and more concise than appending keys then clearing them later, then trying to find and account for all the various exceptions or non-nominal logic paths the code might follow, leaving those context-specific keys to leak outside their intended context.

Make sense? I actually spoke with @dreamorosi about this at re:Invent, if you need more context - maybe he remembers 😆 .

@leandrodamascena
Copy link
Contributor

Hey @jusdino thanks for opening this feature resquest and responding @anafalcao's comment with the experience you expect.

We have 2 other issues related to this (#3719, #2062) and we need to move forward with implementing them.
If you look at other issues, you'll see that there was a thread about the experience we want to achieve when enabling the content manager on a Logger instance: will it be a completely separate/temporary new instance or will it be the same instance just with different keys?

I personally vote for the latter, because I can imagine many scenarios where using the context manager for additional keys could be beneficial, like 1/ transaction workflows, 2/ calling internal functions, and so on. We also support child logger is someone wants to create a separate Logger instance.

That said, I think we need to decide a few things:

1 - What is the name of this new method to use the context manager? People have suggested bind, but I'm not sure that name is very intuitive. Do you have any suggestions?

2 - We also support thread_safe_append_keys, which is thread-safe. AFAIK contextmanager is not thread-safe, so will we only support the standard append_keys method? Any ideas here?

3 - We would love to have your PR here to add this support, can you submit a PR?

Again, thanks a lot for spending time and making the developer experience even better with Powertools.

@jusdino
Copy link
Author

jusdino commented Jan 16, 2025

Oh, I should have looked for those other tickets - my bad.

I agree on the new vs existing instance - modifying the existing instance makes more sense because we want to augment logs within internal functions, without having to send a special logger instance into an internal chain of functions. I'm sure it'll be really common for a dev to realize that they should tack on a new key to a whole branch of their flow after it's been written. Being to simply wrap it into a with block and be done is much preferable to chasing down every single logger reference through a bunch of internal functions, then updating call signatures to pass that new instance around everywhere.

  1. I think something close to what they suggested in #3719 is reasonably intuitive - logger.with_keys(**kwargs). I might lean towards logger.appended_context_keys(**kwargs), since the statement, with logger.appended_context_keys(**kwargs): reads a bit more clearly.

  2. Yeah, I think making this thread safe would really raise the complexity, unless we went through extra hoops in passing new logger instances around or something, which really defeats the purpose of making the safe management of keys more concise.

  3. I could work up a PR for this but it might take me a few weeks to get to it - I have very little free time at the moment.

@anafalcao
Copy link
Collaborator

Hi @jusdino! Thank you for providing such a detailed explanation. The scope of the problem is well defined. And no problem at all, we understand that everyone is facing time constraints at the moment. We will then proceed with creating a PR to address this.

We'll do our best to include this feature in our upcoming release.

If you have any further questions or concerns, please don't hesitate to reach out!

@leandrodamascena leandrodamascena linked a pull request Jan 18, 2025 that will close this issue
7 tasks
@leandrodamascena leandrodamascena removed the triage Pending triage from maintainers label Jan 20, 2025
@leandrodamascena leandrodamascena moved this from Triage to Working on it in Powertools for AWS Lambda (Python) Jan 20, 2025
@github-project-automation github-project-automation bot moved this from Working on it to Coming soon in Powertools for AWS Lambda (Python) Jan 21, 2025
Copy link
Contributor

⚠️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.

@github-actions github-actions bot added the pending-release Fix or implementation already in dev waiting to be released label Jan 21, 2025
@leandrodamascena leandrodamascena moved this from Coming soon to Shipped in Powertools for AWS Lambda (Python) Jan 27, 2025
Copy link
Contributor

This is now released under 3.5.0 version!

@github-actions github-actions bot removed the pending-release Fix or implementation already in dev waiting to be released label Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request feature request need-more-information Pending information to continue
Projects
Status: Shipped
Development

Successfully merging a pull request may close this issue.

3 participants