[Core] Enable Users to Configure Python Standard Log Attributes #49871

MengjinYan · 2025-01-15T22:53:19Z

Why are these changes needed?

This PR enables user to configure a list of additional standard python logging attributes so that the fields can be included in the log recored in for both plain text and structured logging.

It adds an additional field addl_log_std_attrs in the existing ray.LoggingConfig. The addl_log_std_attrs takes in a list of field names and add the fields & its value to the log line when formatting the log line.

Corresponding tests are added as well.

Related issue number

#49502

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

Signed-off-by: Mengjin Yan <[email protected]>

…sible Signed-off-by: Mengjin Yan <[email protected]>

Signed-off-by: Mengjin Yan <[email protected]>

python/ray/_private/ray_logging/formatters.py

python/ray/_private/ray_logging/logging_config.py

jjyao · 2025-01-21T18:47:57Z

python/ray/_private/ray_logging/logging_config.py

+    # Should be marked as @abstractmethod, but we can't do that because of backwards
+    # compatibility.


We don't need to consider backward compatibility here as LoggingConfigurator is not a public thing.

Unfortunately, we need to consider the backward compatibility here because rayturbo extend this class as well.

Then can we just add the extra param to it?

Rayturbo we can change as well so it's not an issue.

That's right. The only thing is if we add the @abstractmethod now, the build will fail after the the PR merged to RayTurbo because the configurator class in RayTurbo doesn't have the method override.

To solve this, I'm planning to have 2 followup PRs:
(1) After the current PR is merged to RayTurbo, I'll update the the configurator class in RayTurbo, override configure_logging_settings function and remove the previous configure_logging function
(2) Update OSS to cleanup the configure_logging function and add @abstractmethod to the configure_logging_settings function to only keep the new function.

In this sense, we can migrate all the configure_logging with configs listed in function parameters to configure_logging_settings function with configs in the data class.

jjyao · 2025-01-21T18:48:05Z

python/ray/_private/ray_logging/logging_config.py

@@ -17,9 +18,17 @@ def get_supported_encodings(self) -> Set[str]:
        raise NotImplementedError

    @abstractmethod
+    @Deprecated


why deprecating this one?

I'm thinking as we add one more field in the config, we should make sure all log fields are configured. So deprecating the previous function that only takes in 2 fields.

jjyao · 2025-01-21T18:48:20Z

python/ray/_private/ray_logging/logging_config.py

+            {
+                "encoding": encoding,
+                "log_level": log_level,
+                "addl_log_std_attrs": [],


what's addl?

addl means "additional". As suggested by Ruiyang above, I'll update to whole word to avoid confusion

python/ray/_private/ray_logging/logging_config.py

…_standard_attrs; (2) add LoggingConfigData to pass the parameters to LoggingConfigurator; (3) Update the new method name in LoggingConfigurator to avoid duplicate function name; (4) Add more commens Signed-off-by: Mengjin Yan <[email protected]>

python/ray/_private/ray_logging/logging_config.py

rynewang · 2025-01-21T21:45:38Z

python/ray/_private/ray_logging/logging_config.py

+class LoggingConfigData:
+    encoding: str
+    log_level: str
+    additional_log_standard_attrs: list


why do we need 2 separate classes: LoggingConfigData and LoggingConfig? I mean, can we just pass the public LoggingConfig to the Configurator?

We can't because otherwise, there will be circular dependency between LoggingConfig and LoggingConfigurator.

Can you tell me more about the circular dependency?

Synced offline. Will leverage the python's forward declaration for types to avoid adding an additional LoggingConfigData class.

python/ray/_private/ray_logging/logging_config.py

Signed-off-by: Mengjin Yan <[email protected]>

jjyao · 2025-01-22T18:58:42Z

python/ray/_private/ray_logging/formatters.py

-        record_format_attrs[LogKey.TIMESTAMP_NS.value] = record_format_attrs.pop(
-            INTERNAL_TIMESTAMP_LOG_KEY
-        )
+class AbstractFormatter(logging.Formatter, ABC):


I don't see any abstract methods

jjyao · 2025-01-22T19:03:53Z

python/ray/_private/ray_logging/logging_config.py

+class LoggingConfigData:
+    encoding: str
+    log_level: str
+    additional_log_standard_attrs: list


Can you tell me more about the circular dependency?

…eak circular depencency; 2. add format as the abstract method in AbstractFormatter Signed-off-by: Mengjin Yan <[email protected]>

Signed-off-by: Mengjin Yan <[email protected]>

MengjinYan added 4 commits January 15, 2025 14:32

enable user to configure python standard log attributes

010a3cf

Signed-off-by: Mengjin Yan <[email protected]>

fix the precommit error; make the log_configure() function more exten…

cad4ce3

…sible Signed-off-by: Mengjin Yan <[email protected]>

fix the test failure

dd33305

Signed-off-by: Mengjin Yan <[email protected]>

Merge branch 'master' into issue-49502

792c8b2

MengjinYan added the go add ONLY when ready to merge, run all tests label Jan 21, 2025

Merge branch 'master' into issue-49502

10bd82f

MengjinYan assigned MengjinYan, jjyao and rynewang and unassigned MengjinYan Jan 21, 2025

MengjinYan requested review from kevin85421, jjyao and rynewang and removed request for kevin85421 January 21, 2025 17:08

rynewang reviewed Jan 21, 2025

View reviewed changes

python/ray/_private/ray_logging/formatters.py Outdated Show resolved Hide resolved

python/ray/_private/ray_logging/logging_config.py Show resolved Hide resolved

jjyao assigned kevin85421 Jan 21, 2025

jjyao reviewed Jan 21, 2025

View reviewed changes

MengjinYan commented Jan 21, 2025

View reviewed changes

python/ray/_private/ray_logging/logging_config.py Show resolved Hide resolved

MengjinYan commented Jan 21, 2025

View reviewed changes

python/ray/_private/ray_logging/logging_config.py Show resolved Hide resolved

rynewang reviewed Jan 21, 2025

View reviewed changes

python/ray/_private/ray_logging/logging_config.py Outdated Show resolved Hide resolved

rynewang reviewed Jan 21, 2025

View reviewed changes

python/ray/_private/ray_logging/logging_config.py Outdated Show resolved Hide resolved

python/ray/_private/ray_logging/logging_config.py Outdated Show resolved Hide resolved

MengjinYan added 5 commits January 21, 2025 14:58

fix review comments

4597710

Signed-off-by: Mengjin Yan <[email protected]>

Merge branch 'master' into issue-49502

61638ec

Merge branch 'master' into issue-49502

6175f59

fix review comment & pre-commit

ef62be2

Signed-off-by: Mengjin Yan <[email protected]>

fix test failure

ab00029

Signed-off-by: Mengjin Yan <[email protected]>

jjyao reviewed Jan 22, 2025

View reviewed changes

MengjinYan added 2 commits January 22, 2025 12:04

fix review comments: 1. Use python forward declaration for type to br…

f31c4e1

…eak circular depencency; 2. add format as the abstract method in AbstractFormatter Signed-off-by: Mengjin Yan <[email protected]>

fix pre-commit failure

93c8bba

Signed-off-by: Mengjin Yan <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Enable Users to Configure Python Standard Log Attributes #49871

[Core] Enable Users to Configure Python Standard Log Attributes #49871

MengjinYan commented Jan 15, 2025 •

edited

Loading

jjyao Jan 21, 2025

MengjinYan Jan 21, 2025

rynewang Jan 21, 2025

jjyao Jan 22, 2025

MengjinYan Jan 22, 2025

jjyao Jan 21, 2025

MengjinYan Jan 21, 2025

jjyao Jan 21, 2025

MengjinYan Jan 21, 2025

rynewang Jan 21, 2025

MengjinYan Jan 21, 2025

jjyao Jan 22, 2025

MengjinYan Jan 22, 2025

jjyao Jan 22, 2025

jjyao Jan 22, 2025

		# Should be marked as @abstractmethod, but we can't do that because of backwards
		# compatibility.

[Core] Enable Users to Configure Python Standard Log Attributes #49871

Are you sure you want to change the base?

[Core] Enable Users to Configure Python Standard Log Attributes #49871

Conversation

MengjinYan commented Jan 15, 2025 • edited Loading

Why are these changes needed?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MengjinYan commented Jan 15, 2025 •

edited

Loading