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

Setup formatted exception to use with async pickling #1046

Closed

Conversation

js-lowes
Copy link

@js-lowes js-lowes commented Dec 16, 2023

Hi thank you for an awesome logging lib! It has made my life 1000% easier...

I am using enqueue=True and I ran into some pickling related issues when it came to 3rd party libraries and their exceptions. I still want to have a stacktrace, but ultimately unless we can pickle the exception, I can't get it downstream in the sink. Ultimately I added an extra field to the RecordException that formats the exception and saves it as string that can be safely pickled.

Overall I think this would solve some of the issues, however it isn't customizable.

I setup this initial PR as a work around to it. I didn't want to sink to much time into until you give some feedback to see if this would be even a good idea and something that would want to be adopted. Would love to clean this up after given some feedback. Please rip into it and let me know!

@js-lowes js-lowes marked this pull request as draft December 28, 2023 15:27
@Delgan
Copy link
Owner

Delgan commented Jan 2, 2024

Hey @js-lowes, thanks for your work on improving Loguru.

I am using enqueue=True and I ran into some pickling related issues when it came to 3rd party libraries and their exceptions.

Could you please elaborate on that?
Since v0.7.1, Loguru wraps the pickle procedures with a safety try / except block capturing a generic Exception. This is precisely to avoid unexpected errors due to third-library exceptions that are not picklable. Therefore, you shouldn't face any issue.

To be fair, I'm not so keen on the idea of extending RecordException with an additional attribute. An "exception" is generally expected to be a tuple of 3 elements, analogous to what is returned by sys.exc_info(). Adding a 4th element would disrupt such assumption and would break API compatibility with traceback.print_exception() for example. Additionally, it may cause confusion and duplication with the formatted exception already available as "{exception}" field in the format configuration of each handler.

If I'm not mistaken, you're trying to access the formatted traceback from your sink, right? What would you think of simply adding it to the record dict through a custom patch or formatter function:

from loguru import logger
import traceback


def save_formatted_exception(record):
    exception = record["exception"]
    if exception is not None:
        formatted_exception = "".join(traceback.format_exception(*exception))
        record["extra"]["formatted_exception"] = formatted_exception


def my_sink(message):
    record = message.record
    print("My custom traceback:", record["extra"]["formatted_exception"])


logger.configure(
    patcher=save_formatted_exception,
    handlers=[{"sink": my_sink, "enqueue": True}],
)

try:
    1 / 0
except Exception:
    logger.exception("Error")

It looks it achieve the functionality you were looking for, without needing to change the Loguru API.

@js-lowes
Copy link
Author

js-lowes commented Jan 2, 2024

Could you please elaborate on that?

We use Elastic Search and Kafka libraries, which throw exceptions that can not be pickled. When leveraging the enqueue=True as documented the record["exception"].value and record["exception"].traceback are both None, so no traceback information can be generated.

To be fair, I'm not so keen on the idea of extending RecordException with an additional attribute. An "exception" is generally expected to be a tuple of 3 elements, analogous to what is returned by sys.exc_info(). Adding a 4th element would disrupt such assumption and would break API compatibility with traceback.print_exception() for example. Additionally, it may cause confusion and duplication with the formatted exception already available as "{exception}" field in the format configuration of each handler.

Yes I totally understand that you don't want the project to be bloated!

If I'm not mistaken, you're trying to access the formatted traceback from your sink, right? What would you think of simply adding it to the record dict through a custom patch or formatter function:

Thanks for the response! Yes, this is a lot better. I did not know about the patcher. I was just debugging the code and I see the patcher gets executed prior to being put onto the internal queue. Thanks for the help I really appreciate it!

Do you think this should be added to the documentation?

@js-lowes js-lowes closed this Jan 2, 2024
@Delgan
Copy link
Owner

Delgan commented Jan 3, 2024

We use Elastic Search and Kafka libraries, which throw exceptions that can not be pickled. When leveraging the enqueue=True as documented the record["exception"].value and record["exception"].traceback are both None, so no traceback information can be generated.

Ok, thanks. That clarifies that there is no unexpected error during logging, but you indeed lose access to the value of the captured exception.

I agree that accessing the formatted error could be useful. Instead of adding a 4th element to RecordException, I've been thinking about implementing a custom __format__() function with the traceback being formatted and cached during initialization. That way, you could retrieve the formatted exception in your sink like that for example:

def sink(message):
    record = message.record
    if record["exception"]:
        formatted_traceback = str(record["exception"])

The problem is that currently the message.record is the same RecordException instance for each handler. However, exception formatting depends on arguments that are handler-specific, such as diagnose=True and backtrace=True. That means I need to make sure each sink receive a different record["exception"] instance, with its own formatting policy. Alternatively, I could deprecate diagnose and backtrace parameters and make them configurable through the logger, not the handler.

To sum up, I don't have a definitive answer regarding this feature, but it's something I'm thinking about.

Thanks for the response! Yes, this is a lot better. I did not know about the patcher. I was just debugging the code and I see the patcher gets executed prior to being put onto the internal queue. Thanks for the help I really appreciate it!

Do you think this should be added to the documentation?

Sure, improving documentation is always welcome. I should indeed add a note about execution order of functions when enqueue=True. Thanks for the feedback.

@js-lowes js-lowes deleted the configure-formatted-exception branch January 3, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants