Skip to content

[Serve] fix missing message body for json log formats - #42729

Merged
edoakes merged 1 commit into
ray-project:masterfrom
GeneDer:fix-empty-log-message-for-json-formatter
Jan 26, 2024
Merged

[Serve] fix missing message body for json log formats#42729
edoakes merged 1 commit into
ray-project:masterfrom
GeneDer:fix-empty-log-message-for-json-formatter

Conversation

@GeneDer

@GeneDer GeneDer commented Jan 26, 2024

Copy link
Copy Markdown
Member

Why are these changes needed?

Found quite a lot of logs misses log message body when working on the logs observability (on Workspaces).
image

After a bit more digging, it seems like in some occasion, the message attribute would not be presented on the record.__dict__ so caused the if statement to be skipped and logged a line without the message body. Internally in LogRecord, the message field is "...computed as msg % args. This is set when Formatter.format() is invoked." This PR uses logging.Formatter to format the message directly instead of relying on Python's string substitution so the message can always be computed. Also did it in the TDD way to ensure there is a test covering it.

LogRecord Doc: https://docs.python.org/3/library/logging.html#logrecord-objects

Related issue number

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: Gene Su <e870252314@gmail.com>
@GeneDer GeneDer self-assigned this Jan 26, 2024
@edoakes

edoakes commented Jan 26, 2024

Copy link
Copy Markdown
Collaborator

@GeneDer so with these changes, what would be the difference in the example screenshot you posted? Would there just be empty message fields instead?

@GeneDer

GeneDer commented Jan 26, 2024

Copy link
Copy Markdown
Member Author

@GeneDer so with these changes, what would be the difference in the example screenshot you posted? Would there just be empty message fields instead?

Yes, the screenshot shows the differences. The first highlighted part is without this change. You see those "Finished recovering deployments...", "Starting proxy with name..." missing. The second run on the bottom shows the fix revealed the missing logs :)

SERVE_LOG_APPLICATION
]

if SERVE_LOG_MESSAGE in record.__dict__:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edoakes Basically this if statement is blocking the message from populating since the formatter is not yet called and the "message" field doesn't exist on the log record yet

@GeneDer

GeneDer commented Jan 26, 2024

Copy link
Copy Markdown
Member Author

@edoakes @sihanwang41 Just in case the notification is missed. Would be nice to get this in 🙏

@edoakes
edoakes merged commit f6da38f into ray-project:master Jan 26, 2024
@GeneDer
GeneDer deleted the fix-empty-log-message-for-json-formatter branch January 26, 2024 22:56
GeneDer added a commit to GeneDer/ray that referenced this pull request Jan 31, 2024
)

Signed-off-by: Gene Su <e870252314@gmail.com>
architkulkarni pushed a commit that referenced this pull request Jan 31, 2024
Pick of #42729

Signed-off-by: Gene Su <e870252314@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants