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: Automatically log exceptions using sys.eventhook #1636

Closed
1 of 2 tasks
lorengordon opened this issue Oct 23, 2022 · 13 comments · Fixed by #1727
Closed
1 of 2 tasks

Feature request: Automatically log exceptions using sys.eventhook #1636

lorengordon opened this issue Oct 23, 2022 · 13 comments · Fixed by #1727
Assignees
Labels

Comments

@lorengordon
Copy link

lorengordon commented Oct 23, 2022

Use case

I find myself often writing a generic try/except statement in my handler to always log all exceptions. I'm not sure if this is a good pattern, maybe it already does this, so I might just be looking for guidance on current practices. (I didn't see anything in the docs or other issues on this.)

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

logger = Logger()


@logger.inject_lambda_context
def handler(event: dict, context: LambdaContext) -> str:
    try:
        return "hello world"
    except Exception:
        logger.exception("Caught an unhandled error")
        raise

Solution/User Experience

What I've done in CLI utilities to capture those exceptions to the logger, is to override the sys.eventhook. This way, every exception is logged automatically. Would it be useful to do something similar for lambda functions?

def exception_hook(exc_type, exc_value, exc_traceback):
    """Log unhandled exceptions with hook for sys.excepthook."""
    log = logging.getLogger(service_name)
    log.exception(
        '',
        exc_info=(exc_type, exc_value, exc_traceback)
    )

sys.excepthook = exception_hook

Alternative solutions

No response

Acknowledgment

@lorengordon lorengordon added feature-request feature request triage Pending triage from maintainers labels Oct 23, 2022
@heitorlessa heitorlessa removed the triage Pending triage from maintainers label Oct 25, 2022
@heitorlessa
Copy link
Contributor

hey @lorengordon, great to hear from you again! I have one question

What I've done in CLI utilities to capture those exceptions to the logger

Are you looking to use this feature in a non-Lambda function (e.g., CLI) ? or are you also wanting to log all exceptions ever raised in a call stack within Lambda?

Python supports Exception Chaining for this particular use case. This allows you have to a complete stack trace and a single exception logged (raise from). Another good use case for Exception Chaining feature is when you raise an internal exception somewhere in your code, and you want to re-raise it with a different name or context, while preserving the original reason why it was raised in the first place.

From Lambda Powertools, logging all exceptions via the sys.eventhook would increase latency slightly (call chain overhead), and more importantly Amazon CloudWatch Logs cost. That's the reason we don't use this hook as it could be intrusive for customers and affect them financially.

If you find this would still be best handled by Powertools, I can label this feature request to hear from more customers as we sort by +1 reaction.

Let me know if that makes sense and if there's any area I can dig deeper!

Thank you for raising this feature request!

@lorengordon
Copy link
Author

lorengordon commented Oct 25, 2022

Hmm, maybe we're misunderstanding each other. The sys.excepthook is only called if an exception is actually raised in a terminating fashion. It would not log every exception in the stack, only the final one. It is perfectly compatible with exception chaining.

With this code, the exception will be captured twice to cloudwatch logs. Once because the logger wrote it (because we want structured logs), and once because Lambda captures the stderr from the raise (which is actually generated by the default sys.excepthook).

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

logger = Logger()


@logger.inject_lambda_context
def handler(event: dict, context: LambdaContext) -> str:
    try:
        raise "Testing exceptions"
    except Exception:
        logger.exception("Caught an error")
        raise

Using a custom sys.excepthook, it is captured only once, by the logger used in the exception hook and so formatted as a structured log, and you do not need try/except just to log the exception and re-raise (which is not usually a great python pattern anyway):

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

logger = Logger()


def exception_hook(exc_type, exc_value, exc_traceback):
    """Log all exceptions with hook for sys.excepthook."""
    logger.exception(
        "%s: %s",
        exc_type.__name__,
        exc_value,
        exc_info=(exc_type, exc_value, exc_traceback),
    )


@logger.inject_lambda_context
def handler(event: dict, context: LambdaContext) -> str:
    raise "Testing exceptions"


sys.excepthook = exception_hook

I guess more accurately, I am asking for a way to simplify that boilerplate. We want to capture a terminating exception as a structured log. But adding the exception_hook method and the sys.excepthook to every lambda takes away from just writing the lambda.

So I thought, maybe there's a way to do this in powertools? Something like this?

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

logger = Logger()


@logger.inject_lambda_context(excepthook=exception_hook)
def handler(event: dict, context: LambdaContext) -> str:
    raise "Testing exceptions"

@heitorlessa
Copy link
Contributor

AH! Completely my bad @lorengordon!! Definitely see value in this as it's uncaught exception.

Two final questions:

  • Should this be a feature flag (env var) and/or a Logger constructor parameter?
  • If I'm reading the doc right, you can have a single exception hook only. Should we accept the fact a customer or 3P library might overwrite it (e.g., Sentry)?
    • Depending on customer demand, we could make it extensible (before/after).

For now, I can see this being a boolean, unless we want to guard against overwrites or make it extensible for 3P we might be unaware. This being an opt-in feature makes me more comfortable either way.

@lorengordon
Copy link
Author

Definitely opt-in. And yes, there is a single exception hook for the program. I have never seen a 3P library try to set it, but I guess anything is possible.

I like the idea of a Logger constructor parameter more than an env var. Feels more flexible. Are you thinking something like this?

logger = Logger(set_excepthook=True)

@heitorlessa
Copy link
Contributor

heitorlessa commented Oct 25, 2022

Yes, we can figure out naming in a PR. Perhaps Logger(log_uncaught_exceptions=True), and in the docstring (and docs) we call out the the hook.

This makes it easier for other Powertools languages to reword exception to error and keep a similar UX ;)

I'm working on a few other issues now, but if you have the bandwidth to send a PR, we can include it in the next release!!

@heitorlessa heitorlessa self-assigned this Oct 26, 2022
@heitorlessa
Copy link
Contributor

another useful trick for this feature: Flush buffered debug logs on uncaught exceptions too.

I think I might have bandwidth to have a go at this in 10 days or so.

@davideicardi
Copy link

I agree on this feature, it can be very useful.

Just a question:
Assuming to use log_uncaught_exceptions=True, every kind of exceptions will be logged, also for example the one derived from ServiceError (NotFoundError, ...)?
I mean what is the meaning of "uncaught"? Uncaught by user code or uncaught by powertools, because I suspect that powertools already catch ServiceError in some way. Right?

My 2 cents:
Probably to have the maximum flexibility we should be able to log in different way based on the exception, for example log NotFoundError as warning ...

thank you!

@heitorlessa
Copy link
Contributor

Great to hear @davideicardi !

As to your question, this is a hook for any exception that was either raised intentionally OR unforeseen. For the ServiceError case, Powertools will catch it and convert into a successful exit ({"statusCode": 500, ...}).

For the flexibility, we could still have a method where you provide your own function to handle any uncaught exception, like set_exception_hook. I initially thought of having a list of exceptions, "allow_list" if you will, but the premise that @lorengordon and Python docs states is primarily for uncaught exceptions -- If you know what the exception is, it's still business as usual since you can catch, log the exception with logger.exception, and decide whether to re-raise or handle gracefully.

For example:

Raised intentionally

from aws_lambda_powertools import Logger
import sys

logger = Logger(service="sample")


def exception_hook(exc_type, exc_value, exc_traceback):
    logger.exception("Unhandled exception", exc_info=(exc_type, exc_value, exc_traceback))


sys.excepthook = exception_hook


def main():
    raise ValueError("Oops!")  # this will crash the program thus calling `sys.excepthook`.


main()

Unexpected/unforeseen exception

from aws_lambda_powertools import Logger
import sys

logger = Logger(service="sample")


def exception_hook(exc_type, exc_value, exc_traceback):
    logger.exception("Unhandled exception", exc_info=(exc_type, exc_value, exc_traceback))


sys.excepthook = exception_hook


def main():
    a = {"key": "value"}
    a["non_existent_key"]  # KeyError will crash the program thus calling `sys.excepthook`.

main()

@heitorlessa
Copy link
Contributor

hmmm turns out it's "impossible" to test this thing. Using it locally works just fine, so I'll ignore test coverage for now, schedule it for this week's release.

I would love to hear from you @lorengordon or @davideicardi if you have ideas on how to test this properly, as I ran out of immediate options.

Working example

from aws_lambda_powertools import Logger

logger = Logger(service="sample", log_uncaught_exceptions=True)

raise ValueError("Catch me if you can!")

Results

{
    "level": "ERROR",
    "location": "log_uncaught_exception_hook:757",
    "message": "Unhandled exception",
    "timestamp": "2022-11-16 11:58:39,819+0100",
    "service": "sample",
    "exception": "Traceback (most recent call last):\n  File \"/Users/lessa/DEV/aws-lambda-powertools-python/blah.py\", line 38, in <module>\n    raise ValueError(\"Catch me if you can!\")\nValueError: Catch me if you can!",
    "exception_name": "ValueError"
}

Testing attempts only to confirm exception hook is never triggered because they are caught so to not fail the test abruptly:

  • Pytest will always catch the exception if I use with pytest.raises(...)
  • Inflicting an uncaught Exception using subprocess via Pipes don't trigger it either

Test attempt with redirect

def test_logger_log_uncaught_exceptions(service_name, stdout):
    # GIVEN Logger is initialized
    Logger(service=service_name, stream=stdout, log_uncaught_exceptions=True)

    from subprocess import Popen, PIPE

    proc = Popen([sys.executable, "non_existed_file.py"], stderr=PIPE)
    _, stderr = proc.communicate()

    # THEN the decorator should included them
    log = capture_logging_output(stdout)

    assert "Unhandled exception" in log["message"]

@github-actions
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 Nov 16, 2022
@heitorlessa
Copy link
Contributor

This should be available by EOW. I spent some time explaining what uncaught exception is to make sure everyone is on the same page - staged docs: https://awslabs.github.io/aws-lambda-powertools-python/develop/core/logger/#uncaught-exceptions

@lorengordon
Copy link
Author

I imagine you could use subprocess with pytest to run a small script that sets up the logger and raises an exception, capture the output, assert that the output contains the expected error?

@github-actions
Copy link
Contributor

This is now released under 2.3.0 version!

@github-actions github-actions bot removed the pending-release Fix or implementation already in dev waiting to be released label Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants