Skip to content

Provide a default value for server_name to LoggingContext #19003

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
+7 −2
Open

Provide a default value for server_name to LoggingContext #19003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d4e7c3
Provide a default value for `server_name`
anoadragon453 Oct 1, 2025
bce2224
re-use newsfile text from #18868
anoadragon453 Oct 1, 2025
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/19003.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix `server_name` in logging context for multiple Synapse instances in one process.
8 changes: 6 additions & 2 deletions synapse/logging/context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,11 @@ class LoggingContext:
Args:
name: Name for the context for logging.
server_name: The name of the server this context is associated with
(`config.server.server_name` or `hs.hostname`)
(`config.server.server_name` or `hs.hostname`).

If not provided, this will be set to "SERVER_NAME_NOT_PROVIDED".
This is a backwards-compatibility patch for
https://github.com/matrix-org/synapse-s3-storage-provider/issues/133.
parent_context (LoggingContext|None): The parent of the new context
request: Synapse Request Context object. Useful to associate all the logs
happening to a given request.
Expand All @@ -309,7 +313,7 @@ def __init__(
self,
*,
name: str,
server_name: str,
server_name: str = "SERVER_NAME_NOT_PROVIDED",
Copy link
Contributor

@MadLittleMods MadLittleMods Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

While, I didn't forsee this problem when making the change, setting a default encourages people not to set this appropriately even in our own Synapse codebase. We could add a lint for our own codebase 🤔

Should downstream consumers really be using LoggingContext? Probably not

This basically the same reasoning I used in #18989 (comment)

[...] we don't expect (and shouldn't support) these things to be used externally.

I would much rather push harder down this route if we're willing (which means this PR isn't necessary).

Copy link
Contributor

@MadLittleMods MadLittleMods Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
server_name: str = "SERVER_NAME_NOT_PROVIDED",
server_name: str = "unknown_server_because_not_provided",

To better match what we have in the codebase:

  • unknown_server_from_sentinel_context
  • unknown_server_from_no_context
  • synapse_module_running_from_unknown_server

We could go even further if we add a lint to enforce server_name is set within Synapse as then we know this only happens from third-party code:

Suggested change
server_name: str = "SERVER_NAME_NOT_PROVIDED",
server_name: str = "unknown_server_from_third_party_code",

This matches the existing synapse_module_running_from_unknown_server:

Suggested change
server_name: str = "SERVER_NAME_NOT_PROVIDED",
server_name: str = "third_party_code_running_from_unknown_server",

We can align on whatever pattern we think is best.

parent_context: "Optional[LoggingContext]" = None,
request: Optional[ContextRequest] = None,
) -> None:
Expand Down
Loading