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.
| /// When no global config has been received from upstream yet, | ||
| /// this will return a default global config. | ||
| pub fn current(&self) -> Arc<GlobalConfig> { | ||
| pub fn current(&self) -> Option<Arc<GlobalConfig>> { |
There was a problem hiding this comment.
I was thinking of adding an additional method. The intention of the global config was always, even if it's missing the defaults should be good enough for Relay to work. For example proxy mode Relays will not have a global config.
But looking at the few places we actually use current() I this might be a better API overall, make it explicit to the caller.
| /// When no global config has been received from upstream yet, | ||
| /// this will return a default global config. |
|
|
||
| // Temporary until we understand why we see false killswitch values sometimes appearing. | ||
| fn default_killswitched() -> bool { | ||
| relay_log::info!("using default killswitched value"); |
There was a problem hiding this comment.
Just a nit:
Can't forget that Relay is a service which is not just used by us, but also by Sentry users. Either self hosted or as an extension of our SaaS infrastructure as managed Relays.
So I think it'd be better if the error was phrased more targeted to the value it targets, along the lines of "using default for endpoint fetch config".
klochek
changed the title
| // Temporary until we understand why we see false killswitch values sometimes appearing. | ||
| fn default_killswitched() -> bool { | ||
| relay_log::info!("using default for endpoint fetch config"); | ||
| bool::default() |
There was a problem hiding this comment.
Bug: The default_killswitched function logs a message on every config deserialization (every 10s) when a field is absent, causing a log flood.
Severity: MEDIUM
Suggested Fix
Remove the info! log statement from the default_killswitched function. Functions used by serde to provide default values should be free of side effects like logging to prevent unintended behavior during frequent deserialization.
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#L100-L103
Potential issue: The `default_killswitched` function emits an `info!` log message every
time it is called. This function is used by `serde` as the default provider for the
`endpoint_fetch_config_enabled` field during deserialization. Because the global config
is fetched every 10 seconds and the field is configured with `skip_serializing_if =
"is_default"`, the field is omitted when its value is `false`. This causes `serde` to
call `default_killswitched` on every fetch cycle, leading to a log flood of
approximately 8,640 messages per relay instance per day in environments where this kill
switch is not explicitly enabled.
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.