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

[Feature] Logger #4688

Closed
wants to merge 16 commits into from
Closed

[Feature] Logger #4688

wants to merge 16 commits into from

Conversation

ArturKnopik
Copy link
Contributor

@ArturKnopik ArturKnopik commented May 16, 2024

Pull Request Prelude

  • I have followed [proper The Forgotten Server code styling][code].
  • I have read and understood the [contribution guidelines][cont] before making this PR.
  • I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

Logger that can handle LUA and C++ calls.

logger.conf - logger config file, not required

#log level (debug, info, warning, error, fatal), default warning
log_level=warning
#max log files, default 2
max_log_files=2
#max size for single log file in bytes, default 1048576(1MB)
log_file_size=1048576
#log file name without ".log", default "tfs"
log_file_name=tfs
#the maximum length of the file address that will be printed in log message, default 35 characters
file_adress_size=35
local logger = GlobalEvent("loggerTest")
function logger.onStartup()
	logD("debug msg")
	logI("info msg")
	logW("warning msg")
	logE("error msg")
	logF("fatal msg")
end
logger:register()

image

Issues addressed:
Lack of true logger.

@@ -338,4 +340,4 @@
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
<ImportGroup Label="ExtensionTargets">
</ImportGroup>
</Project>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

VS autoformater...

@@ -0,0 +1,10 @@
#log level (debug, info, warning, error, fatal), default debug
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix default to warning

@ArturKnopik
Copy link
Contributor Author

todo: missing month

Comment on lines +1167 to +1172
// logger
lua_register(luaState, "logD", LuaScriptInterface::luaLogDebug);
lua_register(luaState, "logI", LuaScriptInterface::luaLogInfo);
lua_register(luaState, "logW", LuaScriptInterface::luaLogWarning);
lua_register(luaState, "logE", LuaScriptInterface::luaLogError);
lua_register(luaState, "logF", LuaScriptInterface::luaLogFatal);
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be a class with some methods

Log.debug("debug")
Log.info("info")
Log.warn("warn")
Log.error("error")
Log.fatal("fatal")

LDEBUG = 0,
LINFO = 1,
LWARNING = 2,
LERROR = 3,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

L prefix is added to shut up VS, it's complaining about ERROR(is treated as maro to something)

src/logger.cpp Outdated Show resolved Hide resolved
@Shawak
Copy link
Contributor

Shawak commented May 16, 2024

I really dislike the fact that it doesn't even prints error messages on stderr. Also the output looks kinda ugly, especially with these _E_, _F_, etc.

I would probably also replace fatal with trace.

@ranisalt
Copy link
Member

What's the purpose of making it a separate thread? I would rather keep it simple and just wrap around Boost.Log, which is already thread-safe.

@ArturKnopik
Copy link
Contributor Author

ArturKnopik commented May 17, 2024

clarification:

  1. why no boost.log/spdlog - I didn't want to add another dependency
  2. message format - I use the format that is used in my work (the project is over 25 years old), it works quite well, this can be changed if someone defines what it should look like
  3. why separated thread: the file open + write operation is expensive, in many cases the file will be opened every time we log, I don't want to block the main thread by writing logs etc.
  4. why a separate configuration file for logger: config.lua is loaded relatively late and I want to have a logger from the very beginning
  5. why logF instead Log.fatal("fatal") etc: I want it to be short and quick to write logs

@ranisalt
Copy link
Member

ranisalt commented May 17, 2024

why separated thread: the file open + write operation is expensive, in many cases the file will be opened every time we log, I don't want to block the main thread by writing logs etc.

It would be better than to write to stdout or stderr and pipe to a file if necessary. So it stays open, managed by the OS

why a separate configuration file for logger: config.lua is loaded relatively late and I want to have a logger from the very beginning

How about using environment variables for that? So you don't need to read any file

@ArturKnopik
Copy link
Contributor Author

It would be better than to write to stdout or stderr and pipe to a file if necessary. So it stays open, managed by the OS

In my opinion, the log size limit is quite significant feature

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