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 3 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
4 changes: 2 additions & 2 deletions rust/src/push/evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ impl PushRuleEvaluator {
Ok(true) => {}
Ok(false) => continue 'outer,
Err(err) => {
warn!("Condition match failed {err}");
warn!(target: "synapse.synapse_rust","Condition match failed {err}");
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder if there's something less intensive than having to write target: ... everywhere.

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 couldn't find a way to set this at like a module / library level, but happy to be proved wrong!

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough. If we get desperate we could possibly make a wrapper macro that looks at the current source file but that's for later if ever :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well apparently I was wrong and it does work fine, see #14976 (comment)

continue 'outer;
}
}
Expand Down Expand Up @@ -209,7 +209,7 @@ impl PushRuleEvaluator {
Ok(true) => true,
Ok(false) => false,
Err(err) => {
warn!("Condition match failed {err}");
warn!( target: "synapse.synapse_rust","Condition match failed {err}");
false
}
}
Expand Down
2 changes: 1 addition & 1 deletion rust/src/push/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ impl PushRules {
2 => push_rules.sender.push(rule),
1 => push_rules.underride.push(rule),
_ => {
warn!(
warn!(target: "synapse.synapse_rust",
"Unrecognized priority class for rule {}: {}",
rule.rule_id, rule.priority_class
);
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()