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

FEAT(client): Introduce new logging system based on spdlog #6707

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

davidebeatrici
Copy link
Member

@davidebeatrici davidebeatrici commented Jan 16, 2025

Sinks:

  • OutputDebugString() (for debugger on Windows)
  • Standard output stream (stdout)
  • File
  • Developer console (Qt widget)

Improvements compared to our old logging system based on Qt:

  • Duplicate messages are skipped if less than 5 seconds have passed. When that happens, an informative message is printed.
  • The previous format/pattern is retained, but the log level (e.g. <W>) is colored based on the severity:
  • The log is now written to a file on all platforms, not only Windows and macOS.
  • When the log file's size reaches 5 MiB, the sink renames it and creates a new one. Up to 4 files are kept:
    Console.txt   -> Console.1.txt
    Console.1.txt -> Console.2.txt
    Console.2.txt -> Console.3.txt
    Console.3.txt -> <delete>
    
  • We can eventually create multiple loggers, ideally one for each facility (Audio, ServerHandler, etc.).
    That would allow us to quickly filter log messages and know right away which subsystem is responsible for them.
  • Modern C++ string formatting. This of course only applies to the new functions such as log::debug(), not qDebug().

Only one "feature" is not reimplemented: showing a dialog in case of a fatal error, right before exiting the program.
However, that only worked on Windows and macOS using their native API, to avoid depending on a Qt event loop:

if (type == QtFatalMsg) {
::MessageBoxA(nullptr, qPrintable(msg), "Mumble", MB_OK | MB_ICONERROR);
exit(0);
}

if (type == QtFatalMsg) {
CFStringRef csMsg = CFStringCreateWithFormat(kCFAllocatorDefault, nullptr, CFSTR("%s\n\nThe error has been logged. Please submit your log file to the Mumble project if the problem persists."), qPrintable(msg));
CFUserNotificationDisplayAlert(0, 0, nullptr, nullptr, nullptr, CFSTR("Mumble has encountered a fatal error"), csMsg, CFSTR("OK"), nullptr, nullptr, nullptr);
CFRelease(csMsg);
exit(0);
}

@davidebeatrici
Copy link
Member Author

For reference, the Windows build failed just for this:

D:/a/1/s/src/mumble/Logger.cpp(55): error C2220: the following warning is treated as an error
D:/a/1/s/src/mumble/Logger.cpp(55): warning C4459: declaration of 'masterSink' hides global declaration
D:/a/1/s/src/mumble/Logger.cpp(30): note: see declaration of 'masterSink'

Sinks:

- OutputDebugString() (for debugger on Windows)
- Standard output stream (stdout)
- File
- Developer console (Qt widget)

Improvements compared to our old logging system based on Qt:

- Duplicate messages are skipped if less than 5 seconds have passed. When that happens, an informative message is printed.
- The previous format/pattern is retained, but the log level (e.g. "<W>") is colored based on the severity.
- The log is now written to a file on all platforms, not only Windows and macOS.
- When the log file's size reaches 5 MiB, the sink renames it and creates a new one. Up to 4 files are kept:
    Console.txt   -> Console.1.txt
    Console.1.txt -> Console.2.txt
    Console.2.txt -> Console.3.txt
    Console.3.txt -> <delete>
- We can eventually create multiple loggers, ideally one for each facility (Audio, ServerHandler, etc.).
  That would allow us to quickly filter log messages and know right away which subsystem is responsible for them.
- Modern C++ string formatting. This of course only applies to the new functions such as log::debug(), not qDebug().

Only one "feature" is not reimplemented: showing a dialog in case of a fatal error, right before exiting the program.
However, that only worked on Windows and macOS using their native API, to avoid depending on a Qt event loop.
Copy link
Member

Choose a reason for hiding this comment

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

Why is the header in a different directory than the implementation file?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be used for the server as well.

Copy link
Member

Choose a reason for hiding this comment

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

No, please don't do that. This is bad design imo. This only calls for macros to be inserted at some point to adapt the header to fit different needs in client and server, which wrecks readability.

If there is shared functionality, then the implementation of that functionality should be shared as well. If needed, you can make a virtual base class where client and server then derive from and implement the specific behavior that needs to differ.

@davidebeatrici davidebeatrici merged commit fb6422a into mumble-voip:master Jan 29, 2025
15 checks passed
@davidebeatrici davidebeatrici deleted the spdlog branch January 29, 2025 03:34
Comment on lines +20 to +22
template< typename... Args > static void inline trace(spdlog::format_string_t< Args... > fmt, Args &&... args) {
spdlog::trace(fmt, std::forward< Args >(args)...);
}
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of wrapping spdlog's functions in our own function when all we do is passing arguments through?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. We can use our own (shorter) and generic namespace (log::trace() instead of spdlog::trace() in this case).
  2. fatal() doesn't exist in spdlog, it's our own function that mimics qFatal()'s behavior (printing the critical error and exiting).
  3. Performance is not affected at all because it's an inline function.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not concerned about performance but maintenance.

The namespace is actually longer as we have mumble::log::* now.

For fatal error, I think it can be argued that there should be a general function fatalError or something like that, that is not in the log namespace. His might make it a bit more clear that this is going to do more than log a message. I think this would improve code readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants