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

Use separate option to control exception formatting #49

Closed
thebigmunch opened this issue Feb 2, 2019 · 21 comments
Closed

Use separate option to control exception formatting #49

thebigmunch opened this issue Feb 2, 2019 · 21 comments
Labels
enhancement Improvement to an already existing feature

Comments

@thebigmunch
Copy link

Currently, the backtrace option is overloaded to control both displaying more of the traceback as well as formatting with better_exceptions. Semantically, those two things should be separate. Having them tied together prevents users from logging just the caught frame with better_exceptions formatting (unless I'm missing something). That's actually how I want most of my exception logging to be.

@Delgan
Copy link
Owner

Delgan commented Feb 2, 2019

Hello @thebigmunch.

Yes you are right, I had this same concern "What if user want to display whole traceback without the variables" (or the opposite), but I didn't know how to handle this in the API.

I guess I could make the backtrace option to either accept a bool or, for advanced users like you, a tuple of bools (True, False) for parametrizing the behavior of exception formatting.

@thebigmunch
Copy link
Author

I guess I could make the backtrace option to either accept a bool or, for advanced users like you, a tuple of bools (True, False) for parametrizing the behavior of exception formatting.

This is a rather poor API design. There you're overloading the value of backtrace as well as the meaning of it. The ideal solution is to add a format_exceptions option to control this separate from backtrace. Loguru is still early in pre-1.0, so it's a great time to make an API change before it becomes a bigger problem.

If you were to overload the option value, you should go with an Enum instead. That would be a clearer API than bool or tuple of bools. You'll get people constantly forgetting which order the tuple represents. But, then you'd be breaking the API as well and should probably just add a separate option.

@thebigmunch
Copy link
Author

After taking a quick look at the code, it may be even better to use the separate option to accept kwargs to pass to the ExceptionFormatter.

What I also see is that the colorize option controls whether to color the exception message, but that's only true if backtrace is True. Otherwise it never makes it to the better_exceptions formatting to be colored. That's quite strange behavior, as well as undocumented. I understand it's the easiest way to work within the current functionality of better_exceptions, but perhaps that could be changed? Or there's some other way to make colorize work regardless of better_exceptions formatting?

@thebigmunch
Copy link
Author

Alright, after having a chance to read all the relevant code, I believe I have the best solution for both controlling the output of variable values as well as the colorization issue.

I think it would be best to add a show_vars (name could be debated) parameter to the ExceptionFormatter in your better_exceptions_fork as well as loguru's Logger. It looks like all that would be needed in better_exceptions_fork is a single if check in format_traceback_frame to decide whether to display those values. Then loguru's ExceptionRecattr.format_exception would just need to be modified to always use ExceptionFormatter with the appropriate arguments.

This way the colorize option controls colorization, the backtrace option controls the depth of the traceback, and the new option (show_vars or whatever is decided) would control the display of variable values.

By the way, I'm willing to do the work on this, if desired.

@thebigmunch
Copy link
Author

For reference, this is what a simple exception looks like with backtrace=False currently:

2019-02-02_06-12-51

This is what it could look like with backtrace=False, show_vars=False:

2019-02-02_06-10-50

And this it could look like with show_vars=True

2019-02-02_06-10-26

Setting colorize=False would remove the color from any of these.

I believe all these options are quite valuable and desirable.

@Delgan
Copy link
Owner

Delgan commented Feb 2, 2019

Disclaimer: I wrote this before reading your last messages

Thanks a lot for helping me investigating the best API! Improvements and suggestions are much welcome, as Loguru did not yet reach v1.0 as you said.

I was afraid of adding another parameter to the .add() function, as there are already many. This is why I proposed the tuple solution. However, you are right, this would be a terrible idea.

Maybe we could go with the backtrace and stackinfo options.

backtrace (bool):
    Whether or not the exception trace should be extended upward, beyond the catching point.

stackinfo (bool):
    Whether or not the exception trace should be improved and display the variables values
    for eases debugging. This should be set to `False` in production to avoid leaking
    sensitive data.

What I also see is that the colorize option controls whether to color the exception message, but that's only true if backtrace is True. Otherwise it never makes it to the better_exceptions formatting to be colored. That's quite strange behavior, as well as undocumented.

The thing is that the colorization of exceptions is handled by the better_exceptions library, so I can't colorize the stack trace if backtrace is False. This is why currently if backtrace is True, you get the whole "improvements" (colorization, upward stack, variable values), otherwise, you get the stock formatting provided by the standard traceback.format_exception() function.
I agree that this can be quite surprising, maybe I could suggest to add a new option to better_exceptions to allow exception colorization without stack variables display.

After taking a quick look at the code, it may be even better to use the separate option to accept kwargs to pass to the ExceptionFormatter.

I agree this may be useful for the user to have more control over exception formatting. Passing kwargs to the option looked interesting first, but finally, I'm not sure I want to go that way.
It risks to become too complex, and will require extra documentation of external libraries. It's too much parametrization (kwargs) inside another parametrization (.add()) in my opinion.
For advanced users needing advanced exception formatting, I would recommend to implement this from within a custom sink or using an appropriate format function (where record["exception"] is available to do whatever user prefer).


About your last messages

So it seems we reached the same conclusion:

  • Add a new parameter to differentiate stack extending / variables info
  • Separate concerns of exception colorization and exceptions variables displaying
    • This requires adding a new parameter to better_exceptions

By the way, I'm willing to do the work on this, if desired.

Your investment is much appreciated, really. ❤️

The thing is that during the last weeks I started to merge some of my changes made in better_exceptions_fork to better_exceptions. Some of these changes are not yet implemented upstream, especially the exception colorization, so for now I can't really suggest this new parameter to the better_exceptions's owner.

This is what it could look like with backtrace=False, show_vars=False:

If backtrace=False, I guess it should not show the File <stdin> ... line, just stop where the exception as been caught (denoted by the > marker).

Also, I think if backtrace and stackinfo (show_vars) are both False, it should be displayed as the standard exception formatting (adding colors eventually), as this has been required by another user (#30).

@thebigmunch
Copy link
Author

thebigmunch commented Feb 2, 2019

Also, I think if backtrace and stackinfo (show_vars) are both False, it should be displayed as the standard exception formatting (adding colors eventually), as this has been required by another user (#30).

And this is another case of overloading options. If you don't want colorize to control the color as a whole or, put another way, you want people to be able to control colorization of the exception separate from the log message, then there should be a separate option for that. Something like colorize_exception. If you had the backtrace/show_vars options control colorization of the exception, loguru would be right back in the same situation my original comment describes, lack of control.

Personally, I think it's fine for colorize to control the colorization of both the log message and exception. It's my gut feeling, that if someone wants the log message colorized, they probably want the exception colorized as well. And if they don't want the log message colorized, they probably don't want exception colorized either. But, I can also see how someone might want to control the colorization of log message and exception separately. So, if that's desired, I strongly urge using a separate option for that as well, possibly renaming the colorize option if deemed confusing.

On the name stackinfo, that doesn't describe what it's doing at all. The stack info is the traceback. What the option is controlling is whether or not to show variable values in the stack info (traceback).

Edit: On the topic of documentation, the exception parameters could be sectioned off like the file sink parameters to maybe ease the parameter list length in the docs.

@Delgan
Copy link
Owner

Delgan commented Feb 2, 2019

And this is another case of overloading options. If you don't want colorize to control the color as a whole or, put another way, you want people to be able to control colorization of the exception separate from the log message, then there should be a separate option for that. Something like colorize_exception. If you had the backtrace/show_vars options control colorization of the exception, loguru would be right back in the same situation my original comment describes, lack of control.

Personally, I think it's fine for colorize to control the colorization of both the log message and exception. It's my gut feeling, that if someone wants the log message colorized, they probably want the exception colorized as well. And if they don't want the log message colorized, they probably don't want exception colorized either. But, I can also see how someone might want to control the colorization of log message and exception separately. So, if that's desired, I strongly urge using a separate option for that as well, possibly renaming the colorize option if deemed confusing.

Oh yeah, I totally agree with you. What I meant is that, if backtrace and show_vars are both False, it should display like this:
2019-02-02_06-12-51
If colorize=True the stacktrace above should be colorized as well, but the stracktrace format should not change besides colors.

On the name stackinfo, that doesn't describe what it's doing at all. The stack info is the traceback. What the option is controlling is whether or not to show variable values in the stack info (traceback).

Oops, I proposed stackinfo as it matched the option in standard logging, but this has not the same purpose, I confused with capture_locals from the traceback module. Using stackinfo is indeed plainly wrong.
I'm open to name suggestions if you have others ideas, otherwise show_vars is quite explicit.

Edit: On the topic of documentation, the exception parameters could be sectioned off like the file sink parameters to maybe ease the parameter list length in the docs.

Yes, absolutely, but I think that backtrace / show_vars would suffice for the vast majority of cases, so there is no need for extra configuration here. On the other hand, I like that exception formatting could be handled with one unique format_exceptions parameter rather than being split in two separate options.

I still don't know which solution I prefer / is the more elegant. We have some time to think about it, waiting to see if better_exceptions_fork can be merged upstream.

@thebigmunch
Copy link
Author

Oh yeah, I totally agree with you. What I meant is that, if backtrace and show_vars are both False, it should display like this:
2019-02-02_06-12-51
If colorize=True the stacktrace above should be colorized as well, but the stracktrace format should not change besides colors.

The reason it currently isn't the case is that with backtrace disabled, the better_exceptions formatter isn't used. This is, in my opinion, something to be fixed.

I agree that the format shouldn't change, but not that it should be the standard Python format. The package is called better_exceptions for a reason. The implementation update I'm thinking of would mean that all the exception formatting would pass through the better_exceptions ExceptionFormatter, using its style. The only way to change that is to change the better_exceptions formatter or write a new formatter to use instead.

On the other hand, I like that exception formatting could be handled with one unique format_exceptions parameter rather than being split in two separate options.

Except this issue is entirely about the fact that it can't be handled by one option. And, after looking deeper, I found that ideally it requires 3 to configure the features loguru wants to expose: colorization, traceback depth, and variable value display. These are 3 very separate concerns each requiring a way to configure.

If you mean that the option would be a dict of kwargs to pass to the ExceptionFormatter, I don't think that's best. I'm much less concerned by the number of parameters there are. If that's the number required to allow what is needed, that's the number the method should have. Now, whether or not the functionality in add could be broken up into other methods that would take the load off is not something I've thought about much. But, at quick glance, that doesn't seem to be the case. Maybe you have some thoughts on that.

I'm open to name suggestions if you have others ideas, otherwise show_vars is quite explicit.

It also mirrors the functionality and intent of the vars builtin : P

PS Didn't say this before, but loguru is the first Python logging library that I've actually enjoyed using to any degree. Everything else is a true nightmare.

@Delgan
Copy link
Owner

Delgan commented Feb 2, 2019

I agree that the format shouldn't change, but not that it should be the standard Python format. The package is called _better__exceptions for a reason. The implementation update I'm thinking of would mean that all the exception formatting would pass through the better_exceptions ExceptionFormatter, using its style. The only way to change that is to change the better_exceptions formatter or write a new formatter to use instead.

What would you expect the format to be if backtrace and show_vars are False?
The second image you posted is my expected output if show_vars=False and backtrace=True. If both are False, there is not much to add except colors.
I think enhancement of the stacktrace thanks to colorization is part of making "better exceptions", so it's ok if passing the stacks to ExceptionFormatter just colorizes the output and nothing more.

Except this issue is entirely about the fact that it can't be handled by one option. And, after looking deeper, I found that ideally it requires 3 to configure the features loguru wants to expose: colorization, traceback depth, and variable value display. These are 3 very separate concerns each requiring a way to configure.

If you mean that the option would be a dict of kwargs to pass to the ExceptionFormatter, I don't think that's best. I'm much less concerned by the number of parameters there are. If that's the number required to allow what is needed, that's the number the method should have. Now, whether or not the functionality in add could be broken up into other methods that would take the load off is not something I've thought about much. But, at quick glance, that doesn't seem to be the case. Maybe you have some thoughts on that.

I don't understand why you think exception colorization requires an extra parameter. As you said, either the user wants colors for its whole logs (including stacktrace), either he won't. So, the exception colorization should depend on the colorize attribute and nothing more. This not currently the case, and this should be addressed.

I was indeed suggesting to pass a kwargs dict that would optionally configure the exception formatting, as you first proposed. However, I think I prefer the backtrace / show_vars solution. There is really nothing much more that need to be configured. For this same reason, I don't think we need to break up this into other methods.

Also, here I'm entirely discussing about the api. I ignore implementation details, if this requires modifying how ExceptionFormatter behaves, I will. That's not a problem.

It also mirrors the functionality and intent of the vars builtin : P

Good point in favor of show_vars then. 🙂

PS Didn't say this before, but loguru is the first Python logging library that I've actually enjoyed using to any degree. Everything else is a true nightmare.

Glad to here that, this means that Loguru reached its point!

@thebigmunch
Copy link
Author

What would you expect the format to be if backtrace and show_vars are False?
The second image you posted is my expected output if show_vars=False and backtrace=True. If both are False, there is not much to add except colors.
I think enhancement of the stacktrace thanks to colorization is part of making "better exceptions", so it's ok if passing the stacks to ExceptionFormatter just colorizes the output and nothing more.

For one, the example chosen actually doesn't show any difference with backtrace=True; it was chosen simply to quickly show the difference with adding a show_vars option.

What you're saying is exactly what I want. When you say format, you may or may not be speaking of something different than I am. When I talk about format, I'm speaking about how the first example, which is not passed to better_exceptions, displays the traceback exactly like it would with standard logging/exception. The second example is the same thing but passed to the better_exceptions formatter, resulting in blank lines being inserted for readability and adding the > where the exception occured. That is, formatting == layout. Color is indeed a separate thing. As is the depth of the traceback. Kinda the point I've been making the entire issue : )

When you said you wanted it like the first one, I thought you meant a colored log message with an uncolored, standard traceback formatting. What will happen in my implementation for your case of backtrace=False, show_vars=False, and colorize=True would actually be the second image.

I don't understand why you think exception colorization requires an extra parameter. As you said, either the user wants colors for its whole logs (including stacktrace), either he won't. So, the exception colorization should depend on the colorize attribute and nothing more. This not currently the case, and this should be addressed.

I don't. In fact, I said I didn't think there needed to be: "Personally, I think it's fine for colorize to control the colorization of both the log message and exception.". But multiple things you said made it sound as if you might've thought otherwise. I said I wasn't against it, in case that was a sticking point. But I think there were just some misunderstandings.

Also, here I'm entirely discussing about the api. I ignore implementation details, if this requires modifying how ExceptionFormatter behaves, I will. That's not a problem.

Yes, that's what I've been talking about, too. I'm big on API design and usability. I've brought up some implementation details as they currently are only because those implementation details are actually part of the problem with the current API (e.g. colorize not doing anything if backtrace=False because it uses a different formatter). That would be fixed for free with this due to the implementation. Also, the implementation for this change is so incredibly simple as to not warrant much discussion itself : )

@thebigmunch
Copy link
Author

To put it in simplest terms, my ideal API proposal was and is:

  • colorize option controls colorization of log message and traceback
  • backtrace option controls depth of traceback
  • show_vars option controls variable value display in traceback

@Delgan
Copy link
Owner

Delgan commented Feb 3, 2019

Ok, so it seems it was mainly an issue of understanding each other. 🙂

Except for one thing that I'm still missing:

What will happen in my implementation for your case of backtrace=False, show_vars=False, and colorize=True would actually be the second image.

Then, what would be your expected output if backtrace=True and show_vars=False?
The > is not where the error occured, it is where it has been caught by the try / except block.
This is acutally not implemented by better_exceptions but on Loguru side:

def _format_catch_point(self, error):

Every frame that is above the > mark is part of a stacktrace that has been re-built by Loguru:
def _extend_traceback(self, tb, decorated):

And this is actually the whole point of backtrace=True: show the entire stacktrace as if the exception had not been caught so user can see the callstack from top to bottom.

To put it in simplest terms, my ideal API proposal was and is:

  • colorize option controls colorization of log message and traceback
  • backtrace option controls depth of traceback
  • show_vars option controls variable value display in traceback

This is what I want too.

@thebigmunch
Copy link
Author

thebigmunch commented Feb 3, 2019

Yeah, I hadn't looked too far into the actual backtrace implementation. I probably didn't have everything modified to display the correct example there (it was early, early AM after a sleepless night). But, the backtrace implementation wouldn't change. It would still control what it controls separate of the better_exceptions formatting and other options.

@Delgan
Copy link
Owner

Delgan commented Feb 3, 2019

Ok, perfect then. Thanks for your patience. 👍

I will implement several changes to better_exceptions_fork, there is some stuff I would like to improve. Then it will be possible to add show_vars to logger.add().

@Delgan
Copy link
Owner

Delgan commented Mar 31, 2019

Hey @thebigmunch. I hope you are recovering little by little and wish this is not too painful. I imagine not being able to code is frustrating, in the meantime here are some updates about exception formatting in Loguru.

I implemented a bunch of modifications about what we discussed and other stuff in #76.

Exceptions coloration now depends solely of the colorize option. The backtrace option only controls the extension of the strack trace. The new diagnose option controls the display of variables values.

I'm not entirely satisfied with the name "diagnose", but I chosen this over "show_vars" because I realized the entire Loguru API uses plain verbs without "_" in it, and I would like to keep this like that. I'm totally open to other suggestions if you have ones, it should ideally be a one-word verb which somehow evokes exceptions formatting ("diagnose" => "something is wrong (the exception), explain me why (the variable values)", but it's dubious I admit it).

@thebigmunch
Copy link
Author

Yeah. I saw the additions as they were being made but haven't tried them out yet. Super excited about vendorizing the exception formatting. I agree that keeping with the style of the rest of the API is fine. I haven't really thought about a better option than diagnose yet. I also feel it isn't quite right but would work if it came down to it.

My computer time is still limited, much of which is taken by my job. But it is looking like I should be getting back to my projects' backlog soon so long as I continue to heed my limits. Yay!

@thebigmunch
Copy link
Author

Some other name options I have come up with to think about: examine or inspect. I think I have seen inspect used for this functionality or similar in an IDE or other programs before.

@thebigmunch
Copy link
Author

Also, inspect would draw a parallel to the stdlib inspect module.

@Delgan
Copy link
Owner

Delgan commented Apr 1, 2019

@thebigmunch Yeah, glad to hear that you are going better. 👍

I had also thought about "inspect" but I was not entirely convinced as I thought it was too general. It describes quite perfectly what the option is doing, but it lacks a conotation related to error management. It's far less abstract than "diagnose", though.

@Delgan
Copy link
Owner

Delgan commented Jun 29, 2019

I kept the diagnose name as I was finally used to it. It's a little cryptic, but I can live with that.

This new argument is now available in the 0.3.0 version just released. 😉

@Delgan Delgan closed this as completed Jun 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to an already existing feature
Projects
None yet
Development

No branches or pull requests

2 participants