[core] Fix gcs logging

[dentiny]

Same motivation as #48931, but different implementation.

TLDR for the problem:

• The excessive logging is caused by bug in setting rotation size in C++ side spdlog, and redirection log from python side doesn't have rotation support
• The proposed solution in this PR is to manage the whole log via spdlog, and disable redirection logic from python

[rynewang]

get_log_file_handles can create names like gcs_server.2.out if gcs_server.out and gcs_server.1.out both exists. Do we have such thing in spdlog?

[dentiny]

All rotated logs are suffixed with id, similar to what you're described.

[rynewang]

can you explain diff of RAY_ROTATION_MAX_BYTES vs FLAGS_log_rotation_size ? If we already have the former, then we only need to fix existing behavior? I see gcs_server_main.cc already call ray::RayLog::StartRayLog and why does the log rotations in it do not work?

[rynewang]

If we don't do anything in this PR, instead only do 2 things:

1. remove python stdout/stderr redirection
2. change ray_log_shutdown_raii from /*log_dir=*/"" to /*log_dir=*/FLAGS_log_dir

will the rotations automatically work?

[jjyao]

Yes but the file name will be changed. We want to keep the existing gcs_server.out filename for backward compatibility.

[dentiny]

will the rotations automatically work?

To answer your question, passing the log directory works for log rotation.
But one motivation would be backward compatibility, namely keep the gcs_server.out filename.

[rynewang]

Now we have 2 ways to specify a RayLog storage:

• log_dir
• log_file

and log_file has higher priority than log_dir.

This setup is a bit nuanced and instead can we do something simpler like this:

1. log_dir controls if spdlog logs or not
2. log_file is relative path (or just a stem name), only used as log file name override

so that:

1. log_dir non-empty, log_name empty -> writes to log_dir by spdlog default log names, subject to duplicate file name renaming and rotations
2. log_dir non-empty, log_name non-empty -> writes to log_dir/log_name by spdlog, subject to duplicate file name renaming and rotations
3. both empty: no spdlog writes
4. log_dir empty, log_name non-empty -> illegal, RAY_LOG(FATAL)

[dentiny]

Now we have 2 ways to specify a RayLog storage:

• log_dir
• log_file

and log_file has higher priority than log_dir.

This setup is a bit nuanced and instead can we do something simpler like this:

1. log_dir controls if spdlog logs or not
2. log_file is relative path (or just a stem name), only used as log file name override

so that:

1. log_dir non-empty, log_name empty -> writes to log_dir by spdlog default log names, subject to duplicate file name renaming and rotations
2. log_dir non-empty, log_name non-empty -> writes to log_dir/log_name by spdlog, subject to duplicate file name renaming and rotations
3. both empty: no spdlog writes
4. log_dir empty, log_name non-empty -> illegal, RAY_LOG(FATAL)

Updated, let me know if I understand correctly.

[rynewang]

why do we have a commented out static?

[dentiny]

Because it's a static function.

[rynewang]

these Case-n are not informative. We can write a prologue like:

We combine log_dir and (log_file or app_name) into a final output file name. Rules:

1. both log_dir and log_file are empty: return "" meaning no log outputs.
1. both log_dir and log_file are NON empty: return f"{log_dir}/{log_name}".
2. log_dir is NON empty, log_file is empty: return f"{log_dir}/{app_name}_{pid}.log".
3. log_dir is empty, log_file is NON empty: check failure.

Then the "Case-n" comments would become meaningful.

[dentiny]

The case-n corresponds to header file.

[dentiny]

Code reference here:

ray/src/ray/util/logging.h

Lines 268 to 273 in c49bae6

	/// A few cases:
	/// 1. If both folder and filename are empty, logging will be displayed to stdout.
	/// 2. If both folder and filename specified, `folder/filename` will be used as output
	/// file.
	/// 3. If only folder filled, default filename by folder is used.
	/// 4. It's illegal to only provide filename.

[rynewang]

ok

[rynewang]

use backticks, not brackets

[dentiny]

Wondering do we have any code style recommendation?

[dentiny]

Or it's the grammar for documentation generator?

[dentiny]

I updated the comment, but still want to know about the coding standing / documentation style.

[rynewang]

no specific style rules, but I often use backticks and never saw brackets in Ray.

[dentiny]

If it's coding consistency I'm fine with it, but backtick is usually used in bash script for execution (i.e. make -jnproc). To reduce mixed usage, I usually use [] for function arguments and referenced function names.