Logging redesign - part 3

[bcoconni]

After some consolidation of the logging code (PR #1141, ##1142 and ##1145), this PR resumes the migration of the code to the new logging system.

I took this opportunity to:

• Rename the member of FGLogging from level to log_level for more clarity and to avoid confusion with the method parameters of FGLogging which tend to be named level as well.
• The minimum level feature has been moved from FGLogger to FGLogConsole as I think the implementation of this feature should not be imposed by the mother class FGLogger.
  • The unit test of FGLogConsole has been extended to check that the minimum logged level behave as intended.
  • The minimum logged level default is set to LogLevel::BULK so that it does not filter anything by default.

There are also a few white spaces changes because I've set my editor to remove trailing spaces before saving. Yes I'm that kind of person who can't resist taking care of this kind of futile details :smile:

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 4.27350% with 112 lines in your changes missing coverage. Please review.

Project coverage is 25.12%. Comparing base (f33f20e) to head (b0e5e52). Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/models/FGAerodynamics.cpp	0.00%	57 Missing :warning:
src/models/FGExternalForce.cpp	0.00%	24 Missing :warning:
src/models/FGAircraft.cpp	0.00%	16 Missing :warning:
src/models/FGBuoyantForces.cpp	0.00%	5 Missing :warning:
src/models/FGFCSChannel.h	0.00%	5 Missing :warning:
src/models/FGAccelerations.cpp	0.00%	3 Missing :warning:
src/models/FGMassBalance.cpp	0.00%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1147      +/-   ##
==========================================
- Coverage   25.13%   25.12%   -0.01%     
==========================================
  Files         170      170              
  Lines       18325    18337      +12     
==========================================
+ Hits         4606     4608       +2     
- Misses      13719    13729      +10     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[seanmcleod]

Looks good.

One follow-up question, partly triggered by https://github.com/JSBSim-Team/jsbsim/issues/834#issuecomment-2305032022 in terms of state reports from notify sections in scripts.

When we update FGScript.cpp do these state reports requested by a user via a script naturally fall into our LogLevel?

https://github.com/JSBSim-Team/jsbsim/blob/f33f20e18f18acb3b55c1c9bb093a6ff5ac08337/src/input_output/FGLog.h#L66-L73

Or are they separate from debug logging, with it's varying levels of debug detail/level?

[bcoconni]

One follow-up question, partly triggered by #834 (comment) in terms of state reports from notify sections in scripts.

When we update FGScript.cpp do these state reports requested by a user via a script naturally fall into our LogLevel?

https://github.com/JSBSim-Team/jsbsim/blob/f33f20e18f18acb3b55c1c9bb093a6ff5ac08337/src/input_output/FGLog.h#L66-L73

Or are they separate from debug logging, with it's varying levels of debug detail/level?

That's a good question.

All messages are meant to be processed by FGLogging to save having several classes. Maybe we could add an additional log level (USER ?) that has a higher priority than all the other log levels to guarantee it is always displayed ?

[bcoconni]

And speaking about #834, I have just committed some changes to replace the occurrences of std::endl by \n. The changes are just applied to the files that were planned to be updated by this PR. The replacement of std::endl in the files that have been modified by the PRs #1094 and #1136 will be addressed in a separate PR

[seanmcleod]

replace the occurrences of std::endl by \n.

Yep, I understand the performance reason, personally just find it more difficult to read the text in the editor with `\n' appended to the last word 😢

Not sure if the following compile time concatenation would be to the liking of other developers 😉

cout << " aircraft config file. (LIFT DRAG)\n";

cout << " aircraft config file. (LIFT DRAG)" "\n";

Maybe we could add an additional log level (USER ?) that has a higher priority than all the other log levels to guarantee it is always displayed?

Hmm, wondering whether we end up needing a bitmask approach?

[bcoconni]

Not sure if the following compile time concatenation would be to the liking of other developers 😉

cout << " aircraft config file. (LIFT DRAG)\n";

cout << " aircraft config file. (LIFT DRAG)" "\n";

I have no strong opinion regarding this. Feel free to modify this PR branch (bcoconni/logging3) to your liking 😄

Maybe we could add an additional log level (USER ?) that has a higher priority than all the other log levels to guarantee it is always displayed?

Hmm, wondering whether we end up needing a bitmask approach?

What do you mean ? Can you detail your thoughts ?

[bcoconni]

I've just pushed another commit to this PR to replace a couple of normint by LogLevel::NORMAL that have been overlooked in PR #1136.

[bcoconni]

I have no strong opinion regarding this. Feel free to modify this PR branch (bcoconni/logging3) to your liking

Or alternatively you can create your own branch and I'll cherry pick your commits to reformat the \n's.

[seanmcleod]

Or alternatively you can create your own branch and I'll cherry pick your commits to reformat the \n's.

Or should I wait for you to finish updating the whole code base to use the new logging mechanism, and then submit a PR that does this reformatting across the whole code base as a single commit?

[bcoconni]

Or alternatively you can create your own branch and I'll cherry pick your commits to reformat the \n's.

Or should I wait for you to finish updating the whole code base to use the new logging mechanism, and then submit a PR that does this reformatting across the whole code base as a single commit?

Sounds good :+1:

[seanmcleod]

Maybe we could add an additional log level (USER ?) that has a higher priority than all the other log levels to guarantee it is always displayed?

Hmm, wondering whether we end up needing a bitmask approach?

What do you mean ? Can you detail your thoughts ?

So we currently have:

https://github.com/JSBSim-Team/jsbsim/blob/e3ac75afabf0a57c343629293ec2c1114e3076be/src/input_output/FGLog.h#L66-L73

With a rough assumption of decreasing verbosity from BULK to FATAL (typically a maximum of 1 of these 😉) , so a JSBSim user/developer gets to set a minimum level via SetMinLevel(LogLevel). Which probably works for the vast majority of uses?

But I was wondering whether a user/developer may want to cherry pick, i.e. via a bitmask, e.g. selecting for example, DEBUG, WARN, ERROR, FATAL.

[bcoconni]

With a rough assumption of decreasing verbosity from BULK to FATAL (typically a maximum of 1 of these 😉) , so a JSBSim user/developer gets to set a minimum level via SetMinLevel(LogLevel). Which probably works for the vast majority of uses?

But I was wondering whether a user/developer may want to cherry pick, i.e. via a bitmask, e.g. selecting for example, DEBUG, WARN, ERROR, FATAL.

I agree that the method FGLogConsole::SetMinLevel() has extremely basic capabilities at filtering the log messages but this can be changed later on.

As you mentioned, different users may have very different needs and for that very reason I think that filtering strategies must not be dictated from the top by the FGLogging class but entirely delegated to the inherited class.

That being said, if we want to allow using a bitmask, I think it's possible to attribute power of 2's values to each element of enum class LogLevel, something like:

 enum class LogLevel { 
   BULK  = 1,  // For frequent messages 
   DEBUG = 2,  // Less frequent debug type messages 
   INFO  = 4,  // Informatory messages 
   WARN  = 8,  // Possible impending problem 
   ERROR = 16, // Problem that can be recovered 
   FATAL = 32  // Fatal problem => an exception will be thrown 
 };

[agodemar]

Will we merge this PR? @bcoconni

[bcoconni]

Will we merge this PR? @bcoconni

Yes, sure. Done ! 😉