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

Log when events are (unexpectedly) filtered out of responses in tests #14213

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/14213.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Log when events are (maybe unexpectedly) filtered out of responses in tests.
3 changes: 2 additions & 1 deletion docker/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ The following environment variables are supported in `generate` mode:
will log sensitive information such as access tokens.
This should not be needed unless you are a developer attempting to debug something
particularly tricky.

* `SYNAPSE_LOG_TESTING`: if set, Synapse will log additional information useful
for testing.

## Postgres

Expand Down
30 changes: 24 additions & 6 deletions docker/conf/log.config
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,35 @@ handlers:
class: logging.StreamHandler
formatter: precise

{% if not SYNAPSE_LOG_SENSITIVE %}
{#
If SYNAPSE_LOG_SENSITIVE is unset, then override synapse.storage.SQL to INFO
so that DEBUG entries (containing sensitive information) are not emitted.
#}
loggers:
# This is just here so we can leave `loggers` in the config regardless of whether
# we configure other loggers below (avoid empty yaml dict error).
_placeholder:
level: "INFO"

{% if not SYNAPSE_LOG_SENSITIVE %}
Copy link
Contributor Author

@MadLittleMods MadLittleMods May 20, 2023

Choose a reason for hiding this comment

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

Should we remove SYNAPSE_LOG_SENSITIVE in favor of SYNAPSE_LOG_TESTING here?

The docs around SYNAPSE_LOG_SENSITIVE already mention it's for "a developer attempting to debug something particularly tricky."

Are we worried about a deprecation period in this case? If it's only a developer thing, I don't think we need to worry about it at all.

{#
If SYNAPSE_LOG_SENSITIVE is unset, then override synapse.storage.SQL to INFO
so that DEBUG entries (containing sensitive information) are not emitted.
#}
synapse.storage.SQL:
# beware: increasing this to DEBUG will make synapse log sensitive
# information such as access tokens.
level: INFO
{% endif %}
{% endif %}

{% if SYNAPSE_LOG_TESTING %}
{#
If Synapse is under test, log a few more useful things for a developer
attempting to debug something particularly tricky.

With `synapse.visibility.filtered_event_debug`, it logs when events are (maybe
unexpectedly) filtered out of responses in tests. It's just nice to be able to
look at the CI log and figure out why an event isn't being returned.
#}
synapse.visibility.filtered_event_debug:
level: DEBUG
{% endif %}

root:
level: {{ SYNAPSE_LOG_LEVEL or "INFO" }}
Expand Down
3 changes: 3 additions & 0 deletions docker/configure_workers_and_start.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@
# log level. INFO is the default.
# * SYNAPSE_LOG_SENSITIVE: If unset, SQL and SQL values won't be logged,
# regardless of the SYNAPSE_LOG_LEVEL setting.
# * SYNAPSE_LOG_TESTING: if set, Synapse will log additional information useful
# for testing.
#
# NOTE: According to Complement's ENTRYPOINT expectations for a homeserver image (as defined
# in the project's README), this script may be run multiple times, and functionality should
Expand Down Expand Up @@ -947,6 +949,7 @@ def generate_worker_log_config(
extra_log_template_args["SYNAPSE_LOG_SENSITIVE"] = environ.get(
"SYNAPSE_LOG_SENSITIVE"
)
extra_log_template_args["SYNAPSE_LOG_TESTING"] = environ.get("SYNAPSE_LOG_TESTING")

# Render and write the file
log_config_filepath = f"/conf/workers/{worker_name}.log.config"
Expand Down
4 changes: 4 additions & 0 deletions scripts-dev/complement.sh
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,10 @@ if [[ -n "$SYNAPSE_TEST_LOG_LEVEL" ]]; then
export PASS_SYNAPSE_LOG_SENSITIVE=1
fi

# Log a few more useful things for a developer attempting to debug something
# particularly tricky.
export PASS_SYNAPSE_LOG_TESTING=1

# Run the tests!
echo "Images built; running complement"
cd "$COMPLEMENT_DIR"
Expand Down
14 changes: 7 additions & 7 deletions synapse/visibility.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
from synapse.util import Clock

logger = logging.getLogger(__name__)

filtered_event_logger = logging.getLogger("synapse.visibility.filtered_event_debug")

VISIBILITY_PRIORITY = (
HistoryVisibility.WORLD_READABLE,
Expand Down Expand Up @@ -97,8 +97,8 @@ async def filter_events_for_client(
events_before_filtering = events
events = [e for e in events if not e.internal_metadata.is_soft_failed()]
if len(events_before_filtering) != len(events):
if logger.isEnabledFor(logging.DEBUG):
logger.debug(
if filtered_event_logger.isEnabledFor(logging.DEBUG):
filtered_event_logger.debug(
"filter_events_for_client: Filtered out soft-failed events: Before=%s, After=%s",
[event.event_id for event in events_before_filtering],
[event.event_id for event in events],
Expand Down Expand Up @@ -319,7 +319,7 @@ def _check_client_allowed_to_see_event(
_check_filter_send_to_client(event, clock, retention_policy, sender_ignored)
== _CheckFilter.DENIED
):
logger.debug(
filtered_event_logger.debug(
"_check_client_allowed_to_see_event(event=%s): Filtered out event because `_check_filter_send_to_client` returned `_CheckFilter.DENIED`",
event.event_id,
)
Expand All @@ -341,7 +341,7 @@ def _check_client_allowed_to_see_event(
)
return event

logger.debug(
filtered_event_logger.debug(
"_check_client_allowed_to_see_event(event=%s): Filtered out event because it's an outlier",
event.event_id,
)
Expand All @@ -367,7 +367,7 @@ def _check_client_allowed_to_see_event(

membership_result = _check_membership(user_id, event, visibility, state, is_peeking)
if not membership_result.allowed:
logger.debug(
filtered_event_logger.debug(
"_check_client_allowed_to_see_event(event=%s): Filtered out event because the user can't see the event because of their membership, membership_result.allowed=%s membership_result.joined=%s",
event.event_id,
membership_result.allowed,
Expand All @@ -378,7 +378,7 @@ def _check_client_allowed_to_see_event(
# If the sender has been erased and the user was not joined at the time, we
# must only return the redacted form.
if sender_erased and not membership_result.joined:
logger.debug(
filtered_event_logger.debug(
"_check_client_allowed_to_see_event(event=%s): Returning pruned event because `sender_erased` and the user was not joined at the time",
event.event_id,
)
Expand Down
12 changes: 12 additions & 0 deletions tests/test_utils/logging_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,16 @@ def setup_logging() -> None:
log_level = os.environ.get("SYNAPSE_TEST_LOG_LEVEL", "ERROR")
root_logger.setLevel(log_level)

# In order to not add noise by default (since we only log ERROR messages for trial
# tests as configured above), we only enable this for developers for looking for
# more INFO or DEBUG.
if root_logger.isEnabledFor(logging.INFO):
# Log when events are (maybe unexpectedly) filtered out of responses in tests. It's
# just nice to be able to look at the CI log and figure out why an event isn't being
# returned.
logging.getLogger("synapse.visibility.filtered_event_debug").setLevel(
logging.DEBUG
)

# Blow away the pyo3-log cache so that it reloads the configuration.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment is just copied from

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

reset_logging_config()