[core] Fix gcs and raylet logging for stdout

[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
• This PR updates logging logic for both GCS and raylet, and only for stdout but not stderr, which is left for next PR

[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.

[jjyao]

I don't see where it's used

[dentiny]

I haven't finished implementation.

[dentiny]

Passing argument is not ideal in a way, since you need to define a priority between env vs passed var.

[jjyao]

Or you can always pass in the final rotation size and calculate on the caller side.

[dentiny]

Yes, updated.

[jjyao]

default_log_rotation_max_size

[dentiny]

google coding stlye suggests constant starts with k and CamelCase: https://google.github.io/styleguide/cppguide.html#Constant_Names

[jjyao]

I mean the parameter name. It's the default value when the env var is not set?

[dentiny]

I won't do that... otherwise you have to define a priority system for pass-ed value and env value.

[jjyao]

@rynewang please merge after CI passes.

[dentiny]

The failed unit test doesn't seem to be related to my change.

[dentiny]

Hi @rynewang , the CI has passed (prev failures are due to known flaky tests), could you please kindly help me review / merge this PR when you have some time? Thank you so much!