-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
server: logs - unified format and --log-format option #5700
Conversation
…log task_id and slot_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what is the benefit of logging the data through json objects and have forgotten why we introduced this in the first place. It makes it difficult to read the log when running in the terminal
We should remove this and log as usual. For example:
LOG_INFO("slot data", {
{"task_id", task.id},
{"n_idle_slots", n_idle_slots},
{"n_processing_slots", n_processing_slots}
});
becomes:
LOG_TEE("%s: slot data - task_id = %d, n_idle_slots = %d, n_processing_slots = %d\n", __func__, task.id, n_idle_slots, n_processing_slots);
@ggerganov I think it (the JSON log) was introduced because you can write like this:
Then the macro will translate the call into:
But unfortunately that such macro never exists in our code base. |
Yeah, in the source code it looks nicer. But in the code you write it once and then in the terminal you read it hundreds of times, so it's better to look nice when you read it |
I've just looked into But that's a bit out-of-scope for this PR, I don't know @ggerganov you prefer to have the log printed out that way? I can propose that in another PR |
No need. We use But all this is low-prio - the PR is good to merge as it is |
I am reverting to the draft, and I will take the time to introduce a |
please have a look to the new approach with --log-format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, but I'd prefer to have --log-format
to be text
by default. I don't think json is preferable, except in the case the log is forwarded to SOC platform like grafanas for example.
Thank you for the hard work, don't forget to take time to relax because it's weekend.
Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
…name] message | additional=data
I would propose to merge this first version, which at the beggining only aims to switch all to the same JSON format. We can introduce colors, output based on level and better C++ code later on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's LGTM. Thanks!
* server: logs - always use JSON logger, add add thread_id in message, log task_id and slot_id * server : skip GH copilot requests from logging * server : change message format of server_log() * server : no need to repeat log in comment * server : log style consistency * server : fix compile warning * server : fix tests regex patterns on M2 Ultra * server: logs: PR feedback on log level * server: logs: allow to choose log format in json or plain text * server: tests: output server logs in text * server: logs switch init logs to server logs macro * server: logs ensure value json value does not raised error * server: logs reduce level VERBOSE to VERB to max 4 chars * server: logs lower case as other log messages * server: logs avoid static in general Co-authored-by: Georgi Gerganov <ggerganov@gmail.com> * server: logs PR feedback: change text log format to: LEVEL [function_name] message | additional=data --------- Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
* server: logs - always use JSON logger, add add thread_id in message, log task_id and slot_id * server : skip GH copilot requests from logging * server : change message format of server_log() * server : no need to repeat log in comment * server : log style consistency * server : fix compile warning * server : fix tests regex patterns on M2 Ultra * server: logs: PR feedback on log level * server: logs: allow to choose log format in json or plain text * server: tests: output server logs in text * server: logs switch init logs to server logs macro * server: logs ensure value json value does not raised error * server: logs reduce level VERBOSE to VERB to max 4 chars * server: logs lower case as other log messages * server: logs avoid static in general Co-authored-by: Georgi Gerganov <ggerganov@gmail.com> * server: logs PR feedback: change text log format to: LEVEL [function_name] message | additional=data --------- Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
Motivation
It becomes difficult to investigate what's going on the server, especially in parallel scenario.
Also, logs are currently not outputting to the same format, making impossible for a logs parser to analyze llama.cpp server logs.
One would expect a human-readable or a JSON log format.
Changes
--log-format FORMAT
: Define the log output to FORMAT: json or text (default: json)LOG_TEE
toLOG_INFO
in almost all places.tid
,slot_id
andtask_id
on all possible logging placesupdate_slots