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

Proposition to add levels to stdlib_logger #261

Merged
merged 17 commits into from
Dec 29, 2020

Conversation

jvdp1
Copy link
Member

@jvdp1 jvdp1 commented Dec 9, 2020

As discussed in #254, I propose to add a level option.
This allows to ignore logging messages: logging messages with a level less severe than the threshold will be ignored.

The threshold values for the different levels must still be discussed (e.g., especially for text_error_level).

WIP:

  • Update the specs.

@jvdp1 jvdp1 changed the title WIP: proposition to add levels to stdlib_logger Proposition to add levels to stdlib_logger Dec 9, 2020
@jvdp1 jvdp1 mentioned this pull request Dec 10, 2020
src/stdlib_logger.f90 Outdated Show resolved Hide resolved
@jvdp1
Copy link
Member Author

jvdp1 commented Dec 12, 2020

I think it is ok for review. @wclodius2 could you have another look, please? Comments from other reviewers would be nice too ;)

@jvdp1
Copy link
Member Author

jvdp1 commented Dec 13, 2020

While I would have error_level and io_error_level have the same severity as they both indicate problems with the application, I would have text_error_level have a different severity as it typically indicates a problem with the contents of an input file. I would have text_error_level have the higher severity. The rest of the edits I think look good.

Thanks @wclodius2 for your comment. I increased the severity level of text_error_level.

Copy link
Member

@ivan-pi ivan-pi left a comment

Choose a reason for hiding this comment

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

I have reviewed the changes introduced by levels. While I am not familiar with the full logger implementation, the changes are clear and I could not spot any errors. The test also appears to cover all of the changes.

Currently the levels are organized into a hierarchy. The other possibility would be to think of levels as switches. Assuming the processor uses a binary representation of integers, the levels can be set as:

none_level = 0
text_error_level = shiftl(1,0) 
io_error_level = shiftl(1,1) ! or the error levels can have the same switch
error_level = shiftl(1,2)
warning_level = shiftl(1,3)
information_level = shiftl(1,4)
debug_level = shiftl(1,5)
all_level = text_error_level + io_error_level + error_level + warning_level + &
                 information_level + debug_level

The decimal values of the levels in the sequence are (0, 1, 2, 4, 8, 16, ...).

The benefit of this representation is that you can selectively turn on only certain combinations by addition of the switches (or subtracting from all_levels). On the other hand this might only be intuitive to users with sufficient knowledge of the underlying binary representation.

I am not saying we need to do it this way, but thought it might be worth discussing what are the pros and cons. The decision we make here will serve as a pattern for similar usage cases appearing in other modules

src/stdlib_logger.f90 Outdated Show resolved Hide resolved
doc/specs/stdlib_logger.md Outdated Show resolved Hide resolved
doc/specs/stdlib_logger.md Outdated Show resolved Hide resolved
@jvdp1
Copy link
Member Author

jvdp1 commented Dec 18, 2020

I have reviewed the changes introduced by levels. While I am not familiar with the full logger implementation, the changes are clear and I could not spot any errors. The test also appears to cover all of the changes.

Currently the levels are organized into a hierarchy. The other possibility would be to think of levels as switches. Assuming the processor uses a binary representation of integers, the levels can be set as:

none_level = 0
text_error_level = shiftl(1,0) 
io_error_level = shiftl(1,1) ! or the error levels can have the same switch
error_level = shiftl(1,2)
warning_level = shiftl(1,3)
information_level = shiftl(1,4)
debug_level = shiftl(1,5)
all_level = text_error_level + io_error_level + error_level + warning_level + &
                 information_level + debug_level

The decimal values of the levels in the sequence are (0, 1, 2, 4, 8, 16, ...).

The benefit of this representation is that you can selectively turn on only certain combinations by addition of the switches (or subtracting from all_levels). On the other hand this might only be intuitive to users with sufficient knowledge of the underlying binary representation.

I am not saying we need to do it this way, but thought it might be worth discussing what are the pros and cons. The decision we make here will serve as a pattern for similar usage cases appearing in other modules

Thank you for your review. Your approach is interesting.
I mainly follow the approach used in Julia and Python loggers.

One of my ideas was to potentially add an option through which the user could customize the different levels. Your approach could then be quite easy because the user could simply specify the rank of the level, instead of an abstract value.

@jvdp1 jvdp1 mentioned this pull request Dec 18, 2020
@jvdp1
Copy link
Member Author

jvdp1 commented Dec 20, 2020

I have reviewed the changes introduced by levels. While I am not familiar with the full logger implementation, the changes are clear and I could not spot any errors. The test also appears to cover all of the changes.
Currently the levels are organized into a hierarchy. The other possibility would be to think of levels as switches. Assuming the processor uses a binary representation of integers, the levels can be set as:

none_level = 0
text_error_level = shiftl(1,0) 
io_error_level = shiftl(1,1) ! or the error levels can have the same switch
error_level = shiftl(1,2)
warning_level = shiftl(1,3)
information_level = shiftl(1,4)
debug_level = shiftl(1,5)
all_level = text_error_level + io_error_level + error_level + warning_level + &
                 information_level + debug_level

The decimal values of the levels in the sequence are (0, 1, 2, 4, 8, 16, ...).
The benefit of this representation is that you can selectively turn on only certain combinations by addition of the switches (or subtracting from all_levels). On the other hand this might only be intuitive to users with sufficient knowledge of the underlying binary representation.
I am not saying we need to do it this way, but thought it might be worth discussing what are the pros and cons. The decision we make here will serve as a pattern for similar usage cases appearing in other modules

Thank you for your review. Your approach is interesting.
I mainly follow the approach used in Julia and Python loggers.

One of my ideas was to potentially add an option through which the user could customize the different levels. Your approach could then be quite easy because the user could simply specify the rank of the level, instead of an abstract value.

@certik @milancurcic @wclodius2 What do you think about @ivan-pi 's proposition?

@milancurcic
Copy link
Member

Thank you for this addition and sorry for the late review. I think levels will be very useful for larger applications and frameworks that use the logger under the hood but allow the user to control the verbosity.

I think @ivan-pi's idea is good as it would allow finer control into which logs exactly to allow and which to suppress. I think the "levels" approach is useful and should go forward as is. Then we should consider to also have a "switches" approach for finer control as a separate PR (but best to first discuss it in the issues).

@jvdp1
Copy link
Member Author

jvdp1 commented Dec 28, 2020

I think @ivan-pi's idea is good as it would allow finer control into which logs exactly to allow and which to suppress. I think the "levels" approach is useful and should go forward as is. Then we should consider to also have a "switches" approach for finer control as a separate PR (but best to first discuss it in the issues).

Thank you @milancurcic for your comments. I'll merge this PR as is later today, and open an issue to discuss the 'switches" approach for finer control.

@jvdp1
Copy link
Member Author

jvdp1 commented Dec 29, 2020

I'll merge this PR. Thank you all for your comments.
I'll open an issues about @ivan-pi 's switches approach for finer control later.

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.

None yet

4 participants