-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Reload the pyo3-log config when the Python logging config changes. #14976
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wonder if there's something less intensive than having to write There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
|
@@ -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 | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,7 @@ | |
from synapse.logging.context import LoggingContextFilter | ||
from synapse.logging.filter import MetadataFilter | ||
from synapse.types import JsonDict | ||
from synapse.synapse_rust import reset_logging_config | ||
|
||
from ..util import SYNAPSE_VERSION | ||
from ._base import Config, ConfigError | ||
|
@@ -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* | ||
|
@@ -237,6 +220,26 @@ def factory(*args: Any, **kwargs: Any) -> logging.LogRecord: | |
|
||
logging.setLogRecordFactory(factory) | ||
|
||
# Configure the logger with the initial configuration. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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() | ||
|
@@ -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: | ||
""" | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 likereset
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.