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

server: logs - unified format and --log-format option #5700

Merged
merged 17 commits into from
Feb 25, 2024

Conversation

phymbert
Copy link
Collaborator

@phymbert phymbert commented Feb 24, 2024

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

  • introduce option --log-format FORMAT: Define the log output to FORMAT: json or text (default: json)
  • Replace usage of LOG_TEE to LOG_INFO in almost all places.
  • add tid, slot_id and task_id on all possible logging places
  • add more verbose logs in update_slots
  • tests are using text based format by default

@phymbert phymbert requested review from ggerganov and ngxson and removed request for ggerganov February 24, 2024 12:17
ngxson
ngxson previously requested changes Feb 24, 2024
examples/server/server.cpp Show resolved Hide resolved
examples/server/server.cpp Outdated Show resolved Hide resolved
examples/server/server.cpp Outdated Show resolved Hide resolved
ggerganov
ggerganov previously approved these changes Feb 24, 2024
Copy link
Owner

@ggerganov ggerganov left a 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);

@ngxson
Copy link
Collaborator

ngxson commented Feb 24, 2024

@ggerganov I think it (the JSON log) was introduced because you can write like this:

                LOG_INFO("slot data", {
                    {"task_id",            task.id},
                    {"n_idle_slots",       n_idle_slots},
                    {"n_processing_slots", n_processing_slots}
                });

Then the macro will translate the call into:

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);

But unfortunately that such macro never exists in our code base.

@ggerganov
Copy link
Owner

@ggerganov I think it (the JSON log) was introduced because you can write like this:

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

@ngxson
Copy link
Collaborator

ngxson commented Feb 24, 2024

I've just looked into server_log(), seems like it's totally possible to "pretty print" json to something like task_id = %d, n_idle_slots = %d, n_processing_slots = %d

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

@ggerganov
Copy link
Owner

No need. We use snprintf-style logging throughout the code base, so at the very least for consistency we should remove the json objects.

But all this is low-prio - the PR is good to merge as it is

@phymbert
Copy link
Collaborator Author

phymbert commented Feb 24, 2024

I am reverting to the draft, and I will take the time to introduce a --log-format option as it will help humans to read logs in the console. But keeping the current format as default for production systems monitoring tools which requires structured log format, json is mainly the standard.

@phymbert phymbert changed the title server: logs - always use JSON logger, add thread_id, slot-id and task_id fields in log message WIP server: logs - unified format Feb 24, 2024
@phymbert phymbert marked this pull request as draft February 24, 2024 16:41
@phymbert phymbert changed the title WIP server: logs - unified format server: logs - unified format and --log-format FORMAT option Feb 24, 2024
@phymbert phymbert marked this pull request as ready for review February 24, 2024 20:12
@phymbert phymbert changed the title server: logs - unified format and --log-format FORMAT option server: logs - unified format and --log-format option Feb 24, 2024
@phymbert phymbert dismissed stale reviews from ggerganov and ngxson February 24, 2024 20:19

please have a look to the new approach with --log-format

Copy link
Collaborator

@ngxson ngxson left a 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.

examples/server/utils.hpp Outdated Show resolved Hide resolved
examples/server/utils.hpp Show resolved Hide resolved
examples/server/utils.hpp Show resolved Hide resolved
examples/server/server.cpp Outdated Show resolved Hide resolved
phymbert and others added 2 commits February 25, 2024 10:49
Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
@phymbert
Copy link
Collaborator Author

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.

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.
@ngxson If you are OK with latest change on the text format. I will merge, thanks for your nice reviews

Copy link
Collaborator

@ngxson ngxson left a 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!

@phymbert phymbert merged commit 930b178 into master Feb 25, 2024
110 checks passed
@phymbert phymbert deleted the feature/server-logs-improvment branch February 25, 2024 12:50
jordankanter pushed a commit to jordankanter/llama.cpp that referenced this pull request Mar 13, 2024
* 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>
hodlen pushed a commit to hodlen/llama.cpp that referenced this pull request Apr 1, 2024
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants