Skip to content

feat(logger): add thread safe logging keys #5141

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

Merged
merged 17 commits into from
Oct 22, 2024

Conversation

SimonBFrank
Copy link
Contributor

@SimonBFrank SimonBFrank commented Sep 8, 2024

Issue number: #991

Summary

Changes

  • New logger methods:
    • append_thread_local_keys : add key to thread-local ContextVar
    • get_current_thread_keys: get all keys from thread-local ContextVar
    • remove_thread_local_keys: remove list of keys from thread-local ContextVar
    • clear_thread_local_keys: remove all keys from thread-local ContextVar
  • Add new logic to_extract_log_keys method to handle error when an additional logging key's value is not a string and the name conflicts with a reserved keys
  • clear_state in inject_lambda_context also clears thread-local keys

User experience

Before:

import concurrent.futures

from aws_lambda_powertools import Logger

logger = Logger(service="payment")


def process_payments(payment_id):
    logger.info("Start processing payment", extra={"payment_id": payment_id})
    ...
    logger.info("End processing payment", extra={"payment_id": payment_id})


with concurrent.futures.ThreadPoolExecutor(max_workers=5) as executor:
    futures = [executor.submit(process_payments, payment_id) for payment_id in range(10)]
    for future in concurrent.futures.as_completed(futures):
        future.result()

After:

import concurrent.futures

from aws_lambda_powertools import Logger

logger = Logger(service="payment")


def process_payments(payment_id):
    logger.clear_thread_local_keys()
    logger.append_thread_local_keys(payment_id=payment_id)
    logger.info("Start processing payment")
    ...
    logger.info("End processing payment")


with concurrent.futures.ThreadPoolExecutor(max_workers=5) as executor:
    futures = [executor.submit(process_payments, payment_id) for payment_id in range(10)]
    for future in concurrent.futures.as_completed(futures):
        future.result()

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?: No, existing functionality isn't broken.RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@SimonBFrank SimonBFrank requested a review from a team September 8, 2024 18:28
@boring-cyborg boring-cyborg bot added documentation Improvements or additions to documentation logger tests labels Sep 8, 2024
Copy link

boring-cyborg bot commented Sep 8, 2024

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 #python channel on our Powertools for AWS Lambda Discord: Invite link

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 8, 2024
@leandrodamascena
Copy link
Contributor

Assigned to @sthulb.

@github-actions github-actions bot added feature New feature or functionality and removed documentation Improvements or additions to documentation labels Sep 9, 2024
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Sep 9, 2024
@SimonBFrank
Copy link
Contributor Author

@sthulb I have made some changes to my PR since the last batch of tests failed due to type checking and | not being supported in Python versions < 3.9. Can you please allow the additional test cases to run?

@sthulb
Copy link
Contributor

sthulb commented Sep 13, 2024

@SimonBFrank We have paused all development and merging in to develop/v3 branches until v3 has gone GA, which is currently scheduled for the 23rd now.

We will review and merge after that. Thank you for this contribution.

@sthulb sthulb changed the base branch from v2 to develop September 25, 2024 08:33
@boring-cyborg boring-cyborg bot added the github-actions Pull requests that update Github_actions code label Sep 25, 2024
@sthulb sthulb changed the base branch from develop to v2 September 25, 2024 08:33
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Sep 25, 2024
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Sep 27, 2024
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Sep 27, 2024
@sthulb
Copy link
Contributor

sthulb commented Sep 27, 2024

This is fine, just got to rebase it into v3

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Oct 2, 2024
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Oct 2, 2024
@SimonBFrank
Copy link
Contributor Author

@leandrodamascena Hey, thanks for the feedback. My responses may be delayed for the next month as my personal life is getting busy.

I think making the logger thread safe by default would not be what I would want as a feature. Let me explain through an example.

I have a Lambda that processes an order that has multiple items. Each item within an order is processed in a separate thread. When I start my function I used append_keys to add the order_id. Then, at the beginning of each thread I use append_thread_local_keys to add the item_id. Example code below:

Code:

import threading
from typing import List

from aws_lambda_powertools import Logger
from aws_lambda_powertools.utilities.typing import LambdaContext

logger = Logger()


def threaded_func(item_id: str):
    logger.append_thread_local_keys(item_id=item_id)
    logger.info("Collecting payment")


def lambda_handler(event: dict, context: LambdaContext) -> str:
    order_id: str = event["order_id"]
    item_ids: List[str] = event["item_ids"]

    logger.append_keys(order_id=order_id)

    for item in item_ids:
        threading.Thread(target=threaded_func, args=(item,)).start()

    return "Success"

Input:

lambda_handler({"order_id": "abc123", "item_ids": ["A1", "A2"]}, {})

Output:

{"level":"INFO","location":"threaded_func:12","message":"Collecting payment","timestamp":"2024-10-02 22:46:16,678-0400","service":"service_undefined","order_id":"abc123","item_id":"A1"}
{"level":"INFO","location":"threaded_func:12","message":"Collecting payment","timestamp":"2024-10-02 22:46:16,678-0400","service":"service_undefined","order_id":"abc123","item_id":"A2"}

This is great because I only need to add the order_id once in the beginning of the Lambda call and will be used everywhere. I can use append_keys as "global" logging keys. But, thread specific information (item_id) can be added to each thread. If we were to make append_keys be thread specific, then we would have to add it in every thread. While this is possible, I find this a lot less user friendly. Additionally, I would imagine making append_keys thread safe would break existing behavior which opens a whole new can of worms. Let me know your thoughts.

@leandrodamascena
Copy link
Contributor

Hi @SimonBFrank! Thanks for the feedback! Last week was very busy and I'll check it out this week to merge the PR.

@SimonBFrank
Copy link
Contributor Author

Hey @leandrodamascena, any updates on getting this reviewed and merged?

@leandrodamascena
Copy link
Contributor

Hey @leandrodamascena, any updates on getting this reviewed and merged?

Hey @SimonBFrank! I've had to deal with some unexpected internal reprioritizations this past week. I dropped the ball a bit here, but tomorrow I'll push some small changes and tests for us to merge, ok?

Thanks

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Oct 17, 2024
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Oct 21, 2024
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Oct 21, 2024
@leandrodamascena
Copy link
Contributor

Update 21/10/2024:

  • New logger methods:
    • thread_safe_append_keys : add key to thread-local ContextVar
    • thread_safe_get_current_keys: get all keys from thread-local ContextVar
    • thread_safe_remove_keys: remove list of keys from thread-local ContextVar
    • thread_safe_clear_keys: remove all keys from thread-local ContextVar
  • Add new logic to_extract_log_keys method to handle error when an additional logging key's value is not a string and the name conflicts with a reserved keys
  • clear_state in inject_lambda_context also clears thread-local keys

@leandrodamascena leandrodamascena self-requested a review October 21, 2024 19:13
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SimonBFrank! Thanks for your patience and hard work on this PR. I sent a commit with some small changes, but basically I renamed the methods and refactored the documentation to make it more specific.

Please let me know if you have any observation before I ask the @sthulb and merge it.

Copy link

@sthulb sthulb merged commit d3698d2 into aws-powertools:develop Oct 22, 2024
10 of 12 checks passed
Copy link

boring-cyborg bot commented Oct 22, 2024

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Oct 22, 2024
@SimonBFrank SimonBFrank deleted the thread-safe-keys branch October 23, 2024 01:48
@SimonBFrank
Copy link
Contributor Author

Hi @SimonBFrank! Thanks for your patience and hard work on this PR. I sent a commit with some small changes, but basically I renamed the methods and refactored the documentation to make it more specific.

Please let me know if you have any observation before I ask the @sthulb and merge it.

Looks amazing to me. I will let me team know that this is ready to use. Thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality github-actions Pull requests that update Github_actions code logger size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Support thread-local keys for logger
3 participants