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

Bug: logger_handler argument is ignored if logging.getLogger() already has a handler. #4277

Open
nostromoJohn opened this issue May 5, 2024 · 14 comments · Fixed by #4295
Open
Assignees
Labels
bug Something isn't working logger revisit-in-3-months Requires more customers feedback before making or revisiting a decision

Comments

@nostromoJohn
Copy link

nostromoJohn commented May 5, 2024

Expected Behaviour

The logger_handler argument for a Logger instance should set the provided logger as the registered handler, regardless of what handlers are already set on logging.getLogger() outputs.

Current Behaviour

When providing a custom logger via the logger_handler argument, the custom log handler is ignored when the logger returned by logging.getLogger(name) already has a handler configured. This is problematic since environments like Serverless Framework cause getLogger to return a logger with a StreamHandler preconfigured.

Code snippet

from logging import Handler
from aws_lambda_powertools import Logger

class CustomHandler(Handler):
    """
    A custom handler class. (Truncated for clarity, contents don't matter)
    """
logger = Logger(logger_handler=CustomHandler())
print(logger.registered_handler) # This prints <StreamHandler <stdout> (NOTSET)>

def handler(event, context):
    print("Lambda running")

Possible Solution

aws_lambda_powertools can override getLogger default handlers, with or without user-provided arguments, instead of assuming the returned logger's handler list is empty.

Steps to Reproduce

  1. Create a custom log handler
  2. Create a Logger instance using the custom handler in the logging_handler parameter.
  3. Deploy and run lambda using Serverless Framework with serverless-python-requirements in a lambda layer config.

Powertools for AWS Lambda (Python) version

latest

AWS Lambda function runtime

3.10

Packaging format used

Lambda Layers

Debugging logs

No response

@nostromoJohn nostromoJohn added bug Something isn't working triage Pending triage from maintainers labels May 5, 2024
Copy link

boring-cyborg bot commented May 5, 2024

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

@nostromoJohn
Copy link
Author

nostromoJohn commented May 5, 2024

I have created a temporary workaround for this issue:

  1. Create a Logger() instance
  2. Remove the preconfigured handler using the private _logger attribute (logger._logger.removeHandler(logger._logger.handlers[0]))
  3. Add the custom handler logger.addHandler(CustomHandler())

@nostromoJohn nostromoJohn changed the title Bug: Custom handler is being ignored when deployed Bug: logger_handler argument is ignored if logging.getLogger already has a handler. May 5, 2024
@nostromoJohn nostromoJohn changed the title Bug: logger_handler argument is ignored if logging.getLogger already has a handler. Bug: logger_handler argument is ignored if logging.getLogger() already has a handler. May 5, 2024
@heitorlessa
Copy link
Contributor

hey @nostromoJohn, thanks a lot for opening up an issue with us -- I'm not sure I fully understood so I'll ask a few questions to help me get a better grasp.

Logger does use the given Logger Handler instead of creating one. However what I'd guess it's happening is the registered_handler property is returning the first available handler -- locally this works fine [bottom], so we need to setup a repro environment with Serverless framework.

Question

  • Could you clarify what Serverless framework is doing, exactly? is that a plugin of sorts?
  • Could you share a minimal reproducible SLS config, if possible? We can try SAM quickly tomorrow, then test with any sample we can create with SLS framework
  • Are you using a custom handler to send data elsewhere and Logger is relying on a parent handler instead?

I haven't used Serverless framework in years so I appreciate your patience in what may seem basic to the non-initiated.

Thanks!


image

@heitorlessa heitorlessa added need-more-information Pending information to continue logger and removed triage Pending triage from maintainers labels May 6, 2024
@heitorlessa heitorlessa moved this from Triage to Pending customer in Powertools for AWS Lambda (Python) May 6, 2024
@heitorlessa heitorlessa self-assigned this May 6, 2024
@nostromoJohn
Copy link
Author

nostromoJohn commented May 7, 2024

Hi @heitorlessa, thanks for you comment! To clarify things, I've recreated the issue without Serverless Framework.
Please take a look at this code snippet (Explanation below)

import os
import logging
from logging import Handler, FileHandler

from aws_lambda_powertools import Logger

SERVICE_NAME = "test_service"
os.environ["POWERTOOLS_SERVICE_NAME"] = SERVICE_NAME


class MyCustomHandler(Handler):
    """
    A custom handler class. Truncated for clarity.
    """

python_logger = logging.getLogger(SERVICE_NAME)

# Running with this line results on the log output written to a file.
# Comment the next line to use MyCustomHandler.
python_logger.addHandler(FileHandler("./log.txt"))

logger = Logger(logger_handler=MyCustomHandler())
logger.critical("URGENT!")

So what is happening here? AWS lambda powertools constructs MyCustomLogger, however it is not used as the handler for the Logger instance. This is due to the underlying python logger already having a configured handler. The code works as intended if we comment out the python_logger.addHandler(FileHandler("./log.txt")) line.
Since Serverless Framework loggers always have a StreamHandler configured, custom handlers do not work (Unless that StreamHandler is explicitly removed, see my previous comment).

I hope that clears this up, let me know if something still isn't clear :)

@heitorlessa
Copy link
Contributor

awesome, THANK YOU for taking this repro 🫶. tackling it after lunch :)

@heitorlessa heitorlessa moved this from Pending customer to Working on it in Powertools for AWS Lambda (Python) May 7, 2024
@heitorlessa heitorlessa removed the need-more-information Pending information to continue label May 7, 2024
@heitorlessa
Copy link
Contributor

heitorlessa commented May 7, 2024

Adding notes as I go through this.

What works as expected

What doesn't work as expected

wip: creating automated tests

  • Logger configures Powertools Formatter to the custom handler
    • Why. It uses registered_handler property to pick up the handler, however right now it returns the first available handler (top-level logger).
    • Why is it a problem?. If an environment or customer intentionally sets up a handler before instantiating a Logger, formatting operations will go to the first handler only -- this is an unusual but it can happen.

@nostromoJohn
Copy link
Author

  • Why. It uses registered_handler property to pick up the handler, however right now it returns the first available handler (top-level logger).

It's important to note that if you print out the handlers property of the Logger instance in the code snippet I provided above - MyCustomHandler will not be inside the handler list. (The output will be [<FileHandler './log.txt'>]) So in this case it's not just a formatter thing, the logger_handler property is just not being pushed into the handlers list.

@heitorlessa
Copy link
Contributor

still working on it - a few meetings in between - will probably finish it tomorrow only.

undocumented and WIP fix: heitorlessa@56b6c2b

@heitorlessa
Copy link
Contributor

Done, lemme know if you think I've missed anything: #4295

I was tempted to solve another suboptimal experience [not a bug] with orphaned child but it's easy to mess up billions of invocations if done wrong.

@nostromoJohn
Copy link
Author

@heitorlessa Awesome! Thank you so much for this

@heitorlessa
Copy link
Contributor

great! Finished cleaning up the docs on a related area (child loggers), as it wasn't clear enough.

Leandro is reviewing this week and we'll include it in the next release by Friday or early next week :)

Thank you one more time for reporting this!

@heitorlessa heitorlessa moved this from Working on it to Pending review in Powertools for AWS Lambda (Python) May 8, 2024
@github-project-automation github-project-automation bot moved this from Pending review to Coming soon in Powertools for AWS Lambda (Python) May 13, 2024
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 May 13, 2024
Copy link
Contributor

This is now released under 2.38.0 version!

@github-actions github-actions bot removed the pending-release Fix or implementation already in dev waiting to be released label May 17, 2024
@heitorlessa heitorlessa moved this from Coming soon to Backlog in Powertools for AWS Lambda (Python) May 18, 2024
@heitorlessa heitorlessa reopened this May 18, 2024
@github-project-automation github-project-automation bot moved this from Backlog to Pending review in Powertools for AWS Lambda (Python) May 18, 2024
@heitorlessa
Copy link
Contributor

reopening as @leandrodamascena had to revert yesterday release as this caused a regression for other customers that our test suite did not cover - we're gonna need time to talk to customers and stress test this more for backwards compatibility

#4362

@heitorlessa heitorlessa moved this from Pending review to Next iteration in Powertools for AWS Lambda (Python) Jun 11, 2024
@dreamorosi dreamorosi added the revisit-in-3-months Requires more customers feedback before making or revisiting a decision label Jul 9, 2024
@dreamorosi dreamorosi moved this from Next iteration to Backlog in Powertools for AWS Lambda (Python) Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working logger revisit-in-3-months Requires more customers feedback before making or revisiting a decision
Projects
Status: Backlog
4 participants