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

Logging cleanup and add options to force long console log format #604

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ulmus-scott
Copy link
Contributor

@ulmus-scott ulmus-scott commented Jul 12, 2022

It was asked on IRC if there was an option to make the console log output include the line numbers, etc. in the last month or so, although I couldn't find it again.

The first part replaces the use of the write system call, which may truncate the output.

The second part adds the new compile-time and run-time options to force the long log format. I am open to other name suggestions for the command-line flag and its corresponding variables.

Also, if there are any preferences for a different long log format, now is the time to change it.

As usual, see commits for details.

Checklist

@ulmus-scott
Copy link
Contributor Author

The format is currently time LOG_LEVEL [pid{/tid}] thread_name __FILE__:__LINE__:__FUNCTION__ message. I would like to add a string version of VB_... after LOG_LEVEL.

Also, I think replacing __FUNCTION__ with __PRETTY_FUNCTION__ (GCC) / __FUNCSIG__ (MSVC) might be beneficial.

@paul-h
Copy link
Contributor

paul-h commented Jul 16, 2022

It probably goes without saying but we need to be careful not to add too much extra processing of each log message because with the debug level enabled the logs can get very verbose and if you don't have a supercomputer then the logging just bog down the system making it harder to debug the problem you are trying to look at.

@ulmus-scott
Copy link
Contributor Author

As the changes currently stand, the FileLogger has an extra function call to generate the log message because it is not inline, but I don't consider that a noticeable increase because of the two QString function calls which malloc.

logConsole() has an extra if statement.

__FUNCTION__ versus __PRETTY_FUNCTION__ / __FUNCSIG__ is just a different const char* the compiler has already created.

Adding the LOG_LEVEL would, in my opinion, be useful for narrowing down what verbose arguments are needed when starting from all or most. However, this would involve a large number of independent if statements (64?) because it is a bitset, not an enumeration.

@ulmus-scott
Copy link
Contributor Author

Rebased onto master without change to force rebuild.

@ulmus-scott
Copy link
Contributor Author

Rebased onto master and fixed a typo in a Doxygen comment.

@twitham1
Copy link
Contributor

I have found it challenging to discover where in the huge code base certain things happen. Seems like this feature could make that much easier. I might not want larger logs in production but it would be a nice option when trying to find a bug or feature to tweak.

@ulmus-scott
Copy link
Contributor Author

Rebased onto master. I don't know what would be necessary to always force the long log format when using CMake.

ulmus-scott and others added 8 commits October 7, 2024 01:43
by replacing the write system call, which may only partially write,
with the C++ standard iostreams.
which it doesn't need to be.
The POSIX write() system call may truncate the output.  The proper
way to use it is in a loop.

Instead, use the high level C++ STL I/O.

The Boost library would be needed to keep using the open() and close()
system calls, i.e. to wrap a FILE* with an iostream.

I don't see a way via the C++ STL ofstream to set the Unix file
permissions.  However, it seems to default to the previously
specified 0664.

I'm not sure how to test that FileLogger::reopen() actually works
with a log roller, but it should be equivalent.
between console and file logging.

The new shared long log format is the logConsole format.
Initially for compatibility with FFmpeg's log levels, but should also be
useful in MythTV.
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.

4 participants