Skip to content

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Oct 1, 2025

This is a backwards-compatibility patch for modules that imported LoggingContext from within Synapse. See matrix-org/synapse-s3-storage-provider#133.

Spawned from a change in #18868, which has not yet gone out in a release. As such, re-uses the changelog from it.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct (run the linters)

This is a backwards-compatibility patch for modules that imported LoggingContext from within Synapse. See
matrix-org/synapse-s3-storage-provider#133
@anoadragon453 anoadragon453 marked this pull request as ready for review October 1, 2025 14:20
@anoadragon453 anoadragon453 requested a review from a team as a code owner October 1, 2025 14:20
@anoadragon453 anoadragon453 changed the title Provide a default value for server_name Provide a default value for server_name to LoggingContext Oct 1, 2025
*,
name: str,
server_name: str,
server_name: str = "SERVER_NAME_NOT_PROVIDED",
Copy link
Contributor

@MadLittleMods MadLittleMods Oct 1, 2025

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).

*,
name: str,
server_name: str,
server_name: str = "SERVER_NAME_NOT_PROVIDED",
Copy link
Contributor

@MadLittleMods MadLittleMods Oct 1, 2025

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.

@anoadragon453
Copy link
Member Author

This PR is paused while an alternative solution is explored in matrix-org/synapse-s3-storage-provider#134.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants