-
Notifications
You must be signed in to change notification settings - Fork 231
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
Revert "Introduce spdlog logging system" #271
Conversation
This reverts commit b0e5754.
I am on board for whatever decision everyone ends up agreeing on. |
What was the motivation for using spdlog to begin with? |
|
OK. If I’m understanding that thread correctly, spdlog was added in order to make it easier to implement these three features:
Did I get that right? I ask because reading #248 felt like reading inside baseball to me. |
Command line flags is just one idea on how to control the logs, the log system is unaware of this stuff. It is very common today that software logs to a logger construct rather directly to a stream, and then backend loggers can be customized on what they do with the logging, like dump it to rotating log files or send it up a network server, etc. Don't forget formatting of the log messages can be used, even separately per logger backend (eg: colored text for terminals). It seems lately, people only think why bother with a logging framework on top standard output streams, but its there for collecting and organizing data. The Android OS provides a built in logging framework that allow logs to be sent to a server when an app bug is reported. When logging through spdlog's provided macros, they can capture the source file and line number, so know where exactly these log messages came from. No one has to guess anymore on what might be the problem is, at least we can log out better clues, at least for D3 builds running without the debugger, still the debugger only breaks when a breakpoint is hit or some signal is trapped, and still you do not get human friendly information from that. With logging, we could log out the details of how the mission file was loaded and setup. Trace log level is for the nitty gritty stuff where you may dump large amount of data as a string, it is never recommended to use that in release bugs, trace logs are allowed to slow down the entire process for the sake of capturing a high volume amount detailed data when someone enables it explicitly at build time. Here is a response from ChatGPT Here’s why trace logging is often disabled at compile time in production environments: Performance Impact: Trace logging can significantly affect the performance of an application. Because it captures detailed information about many aspects of the application, it can lead to large amounts of data being written to log files. This process consumes I/O resources and can slow down the application, especially if the logging is not optimized or the I/O subsystem is already under heavy load. |
OK, so here’s a new list based on #248 and on the information that you just gave me. The motivation for having spdlog is to make it easier to implement these features:
Did I understand correctly? |
systemd journal is just for systemd services not standalone applications. Trace level in general just means a lot of detailed stuff can be logged, it could be used anywhere. I always use compile time flags to control the logging, but it seems other want to use a simple cli verbosity flag to control them as well. I think -logging is all we need, to simply turn on logging to file, which can be just info, error and warn. |
I think that you misunderstood what I was trying to say. I said “The ability to output logs + metadata to something like
OK. What if I changed my previous statement to this:
Would that be a more accurate reflection? |
Gotcha. Read about Log4j, --priority is similar to the log verbosity stuff for spdlog, if you were to control it via command line. |
At the moment, there’s no real reason to have a custom log function. I only added a custom log function to make it easier to create future commits that will implement proof-of-concept versions of these features [1]. [1]: <DescentDevelopers/Descent3#271 (comment)>
I took the list of features that I had mentioned previously, and I tried to implement them myself in an example project. In the example project, I used SDL’s logging API. I did so to try and see if SDL’s logging API is suitable for our purposes. Here’s what I learned from trying to implement each of the features on my list:
For my example implementation, I decided to actually In order to get SDL’s logging API to send logs to
Unfortunately, SDL’s logging API and systemd’s logging API were kind of incompatible here. For SDL’s logging API, I had to write a function with this signature: void CustomLogOutputFunction(void *userdata, int category,
SDL_LogPriority priority, const char *message); The problem here is int sd_journal_print_with_location(int priority,
const char *file,
const char *line,
const char *func,
const char *format,
...); By the time a messages gets passed to an
This was easy to implement. All I had to do was add a little bit of code to my custom
SDL’s logging API gives us this for free. Specifically, you can set the
SDL calls this log level “verbose”. I haven’t tried it myself, but I think that you could create a preprocessor macro that calls In summary, it seems like we can use SDL’s logging API to implement everything that we want to implement with minimal effort. All we have to do is:
The only thing that we can’t do easily is output logs to some sort of database that has fields specifically for filenames and line numbers. It is certainly possible to do that, it’s just not easy. If we were to stick with spdlog, then we would still need to write a custom My conclusion is that we should get rid of spdlog in favor of using SDL’s logging API. It seems wasteful to have two independent logging mechanisms for a single application. When you consider the fact that we’ll have to write a custom |
Alright, this logging situation has dragged on for too long, it's starting to become ridiculous. There is so much other areas to improve D3 on, let's not waste anymore time on this issue. #341 replaces this PR and #327, removes the spdlog dependency while keeping the logging module if we want to improve on SDL2's. |
This reverts commit b0e5754.
I'm not against a better logging system, but introducing an external dependency for it seems like a wrong move, especially since it adds a bunch of complexity to write text to stdout.
If we're dead-set on keeping spdlog, please close this PR and we can start a new one that removes the external dependency, using their header-only version instead included in this repo, so people don't need to track down the library from elsewhere.
But really, I don't think we should keep this at all.
(Also, SDL2 has a
SDL_Log
function which does the right thing even on platforms without stdout, if that was the reason for choosing spdlog, so in a future where the Windows builds also use SDL, that might be a viable solution, but I'm not specifically advocating for that with this PR.)