feat(upload): add global config default logging#6019
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| fn global_config_default() -> GlobalConfig { | ||
| relay_log::info!("using default global config"); | ||
| GlobalConfig::default() | ||
| } |
There was a problem hiding this comment.
Bug: The #[serde(default)] on GlobalConfig causes global_config_default() to log a message on every normal config update, creating excessive log noise.
Severity: LOW
Suggested Fix
Remove the logging from global_config_default(). If logging for unexpected defaults is required, it should be implemented in a way that distinguishes between a truly empty/default config and a partial config update, which is a normal operational state. For example, check if the entire configuration object is missing, not just individual fields.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-dynamic-config/src/global.rs#L16-L19
Potential issue: The `global_config_default()` function is intended to log when
unexpected default values are used. However, due to the `#[serde(default = "fn")]`
attribute on the `GlobalConfig` struct, this function is executed on every
deserialization where any field is missing. Since several fields are annotated with
`skip_serializing_if`, they are frequently absent from production JSON payloads. This
results in a log message being generated every 10 seconds during steady-state operation,
creating useless log noise and obscuring meaningful diagnostics.
Did we get this right? 👍 / 👎 to inform future reviews.
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
There is also an option of adding more debug/trace level logs which are generally useful to debug issues and then specifically turn them on in our test/canary/prod environments.
| /// [`ProjectConfig`](crate::ProjectConfig)s small. | ||
| #[derive(Default, Clone, Debug, Serialize, Deserialize)] | ||
| #[serde(default, rename_all = "camelCase")] | ||
| #[serde(default = "global_config_default", rename_all = "camelCase")] |
There was a problem hiding this comment.
I don't think this will work like you'd want it to. When individual fields are missing, serde will construct a default of the struct to fill in the missing parts. This can happen on every request. Playground.
There was a problem hiding this comment.
Yikes, I did not know this. Okay--using a per-field default works as desired, tested here
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 80e018e. Configure here.
| fn default_killswitched() -> bool { | ||
| relay_log::info!("using default killswitched value"); | ||
| bool::default() | ||
| } |
There was a problem hiding this comment.
Debug log fires on normal deserialization, not just errors
Low Severity
The default_killswitched function logs every time relay.endpoint-fetch-config.enabled is absent during deserialization — which is the normal case when the upstream hasn't configured this option. It fires every fetch cycle (~10 seconds) regardless of whether anything is actually wrong, making it unable to distinguish the "false killswitch values" bug from routine behavior. As noted in the PR discussion, this won't achieve the intended debugging goal.
Reviewed by Cursor Bugbot for commit 80e018e. Configure here.
2 participants
Still not sure of the underlying cause, but this might help clear up whether we're getting default values during some serialization layer.