Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Reload the pyo3-log config when the Python logging config changes. #14976

Merged
merged 4 commits into from
Feb 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/14976.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug introduced in Synapse 1.68.0 where logging from the Rust module was not properly logged.
17 changes: 15 additions & 2 deletions rust/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
use lazy_static::lazy_static;
use pyo3::prelude::*;
use pyo3_log::ResetHandle;

pub mod push;

lazy_static! {
static ref LOGGING_HANDLE: ResetHandle = pyo3_log::init();
}
Comment on lines +7 to +9
Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea if this is safe / smart to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine! The Rust module is loaded after Python is started up, so can't see that being a problem.

And unless you write unsafe, I generally trust the Rust compiler to know what it's talking about with thread safety — it looks like reset takes (&self) and you're not mutating this afterwards, plus the Python GIL means it's all going to be single-threaded anyway ... I can't see why this should be concerning.


/// Returns the hash of all the rust source files at the time it was compiled.
///
/// Used by python to detect if the rust library is outdated.
Expand All @@ -17,13 +23,20 @@ fn sum_as_string(a: usize, b: usize) -> PyResult<String> {
Ok((a + b).to_string())
}

/// Reset the cached logging configuration of pyo3-log to pick up any changes
/// in the Python logging configuration.
///
#[pyfunction]
fn reset_logging_config() {
LOGGING_HANDLE.reset();
}

/// The entry point for defining the Python module.
#[pymodule]
fn synapse_rust(py: Python<'_>, m: &PyModule) -> PyResult<()> {
pyo3_log::init();

m.add_function(wrap_pyfunction!(sum_as_string, m)?)?;
m.add_function(wrap_pyfunction!(get_rust_file_digest, m)?)?;
m.add_function(wrap_pyfunction!(reset_logging_config, m)?)?;

push::register_module(py, m)?;

Expand Down
1 change: 1 addition & 0 deletions stubs/synapse/synapse_rust/__init__.pyi
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
def sum_as_string(a: int, b: int) -> str: ...
def get_rust_file_digest() -> str: ...
def reset_logging_config() -> None: ...
42 changes: 24 additions & 18 deletions synapse/config/logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

from synapse.logging.context import LoggingContextFilter
from synapse.logging.filter import MetadataFilter
from synapse.synapse_rust import reset_logging_config
from synapse.types import JsonDict

from ..util import SYNAPSE_VERSION
Expand Down Expand Up @@ -200,24 +201,6 @@ def _setup_stdlib_logging(
"""
Set up Python standard library logging.
"""
if log_config_path is None:
log_format = (
"%(asctime)s - %(name)s - %(lineno)d - %(levelname)s - %(request)s"
" - %(message)s"
)

logger = logging.getLogger("")
logger.setLevel(logging.INFO)
logging.getLogger("synapse.storage.SQL").setLevel(logging.INFO)

formatter = logging.Formatter(log_format)

handler = logging.StreamHandler()
handler.setFormatter(formatter)
logger.addHandler(handler)
else:
# Load the logging configuration.
_load_logging_config(log_config_path)

# We add a log record factory that runs all messages through the
# LoggingContextFilter so that we get the context *at the time we log*
Expand All @@ -237,6 +220,26 @@ def factory(*args: Any, **kwargs: Any) -> logging.LogRecord:

logging.setLogRecordFactory(factory)

# Configure the logger with the initial configuration.
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this has to be moved, but since our logging generally assumes that a request exists on the Record it makes sense to set that up first, in case something quickly logs.

if log_config_path is None:
log_format = (
"%(asctime)s - %(name)s - %(lineno)d - %(levelname)s - %(request)s"
" - %(message)s"
)

logger = logging.getLogger("")
logger.setLevel(logging.INFO)
logging.getLogger("synapse.storage.SQL").setLevel(logging.INFO)

formatter = logging.Formatter(log_format)

handler = logging.StreamHandler()
handler.setFormatter(formatter)
logger.addHandler(handler)
else:
# Load the logging configuration.
_load_logging_config(log_config_path)

# Route Twisted's native logging through to the standard library logging
# system.
observer = STDLibLogObserver()
Expand Down Expand Up @@ -294,6 +297,9 @@ def _load_logging_config(log_config_path: str) -> None:

logging.config.dictConfig(log_config)

# Blow away the pyo3-log cache so that it reloads the configuration.
reset_logging_config()


def _reload_logging_config(log_config_path: Optional[str]) -> None:
"""
Expand Down
3 changes: 3 additions & 0 deletions tests/test_utils/logging_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import twisted.logger

from synapse.logging.context import LoggingContextFilter
from synapse.synapse_rust import reset_logging_config


class ToTwistedHandler(logging.Handler):
Expand Down Expand Up @@ -52,3 +53,5 @@ def setup_logging():

log_level = os.environ.get("SYNAPSE_TEST_LOG_LEVEL", "ERROR")
root_logger.setLevel(log_level)

reset_logging_config()