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

Dont log message contents #1862

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rukai
Copy link
Member

@rukai rukai commented Dec 12, 2024

This PR removes all cases that could possibly lead to logging message contents:

  • The first and most important change is to remove trace level logs from release binaries. This is done by adding the release_max_level_debug feature. This allows trace level logs to log message contents for debugging purposes while making it impossible for these logs to make it into production.
  • Move a few debug level logs to trace level logs since they include message contents.
  • Remove message contents from some errors that include values, these errors needed to be combined with another bug in shotover or a bug in the client or DB for message contents to be logged. This PR removes that possibility entirely.
  • the vast majority of the PR is just minor syntax cleanup of moving arguments inline with the format string, since I was auditing every log it made sense to do at the same time. These kinds of cleanups make such audits easier to perform in the future.

None of the issues I found were severe enough to warrant an immediate patch release. Since it would take an unusual configuration or another bug in shotover for message contents to be logged.

Copy link

codspeed-hq bot commented Dec 12, 2024

CodSpeed Performance Report

Merging #1862 will not alter performance

Comparing rukai:dont_log_message_contents (70fc86d) with main (026327f)

Summary

✅ 38 untouched benchmarks

@rukai rukai force-pushed the dont_log_message_contents branch 3 times, most recently from 95ca0d7 to 6c4b863 Compare December 13, 2024 00:52
@rukai rukai force-pushed the dont_log_message_contents branch from 6c4b863 to 70fc86d Compare December 13, 2024 01:03
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