-
Notifications
You must be signed in to change notification settings - Fork 392
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.
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
base: develop
Are you sure you want to change the base?
Provide a default value for server_name
to LoggingContext
#19003
Conversation
This is a backwards-compatibility patch for modules that imported LoggingContext from within Synapse. See matrix-org/synapse-s3-storage-provider#133
server_name
server_name
to LoggingContext
*, | ||
name: str, | ||
server_name: str, | ||
server_name: str = "SERVER_NAME_NOT_PROVIDED", |
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.
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", |
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.
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:
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
:
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.
This PR is paused while an alternative solution is explored in matrix-org/synapse-s3-storage-provider#134. |
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
EventStore
toEventWorkerStore
.".code blocks
.