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

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

Merged

Conversation

GeneDer
Copy link
Contributor

@GeneDer GeneDer commented Jan 26, 2024

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
Copy link
Contributor

edoakes commented Jan 26, 2024

@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
Copy link
Contributor Author

GeneDer commented Jan 26, 2024

@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 :)

@@ -81,10 +81,10 @@ def format(self, record: logging.LogRecord) -> str:
SERVE_LOG_APPLICATION
]

if SERVE_LOG_MESSAGE in record.__dict__:
Copy link
Contributor 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
Copy link
Contributor Author

GeneDer commented Jan 26, 2024

@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
9 checks passed
@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.

None yet

2 participants