feat(server): Return global config status for downstream requests #2765
Conversation
| let mut pending = Vec::with_capacity(keys_len); | ||
| let mut configs = HashMap::with_capacity(keys_len); |
There was a problem hiding this comment.
i think its better when variables are declared closer to where theyre being used
| /// Instead of using [`Status`], we use StatusResponse as a separate field in order to not | ||
| /// make breaking changes to the api. |
There was a problem hiding this comment.
nit: I think the context this comment provides is better suited in a GitHub PR comment rather than code comment.
There was a problem hiding this comment.
I changed it to normal comment rather than doc comment, cause I think it's not directly intuitive why we do it like this, so its nice for reader to know it's because of API
| @@ -40,6 +40,23 @@ type UpstreamQueryResult = | |||
| struct GetGlobalConfigResponse { | |||
| #[serde(default)] | |||
| global: Option<GlobalConfig>, | |||
| // Instead of using [`Status`], we use StatusResponse as a separate field in order to not | |||
| // make breaking changes to the api. | |||
| global_status: Option<StatusResponse>, | |||
There was a problem hiding this comment.
Doesn't this also need a #[serde(default)] attribute (like global above)?
There was a problem hiding this comment.
huh id think that was necessary yeah, but it's weird integration tests didnt fail because of that, since minisentry don't return a global_status
There was a problem hiding this comment.
We could remove the top #[serde(default)], serde implicitly has the default behaviour for Option's, they are special in that sense.
| @@ -256,8 +265,12 @@ impl GlobalConfigService { | |||
| match result { | |||
| Ok(Ok(config)) => { | |||
| let mut success = false; | |||
| // Older relays won't send a global status, in that case, we will pretend like the | |||
| // default global config is an up to date one, because that was the old behaviour. | |||
| let is_ready = config.global_status.map_or(true, |stat| stat.is_ready()); | |||
There was a problem hiding this comment.
nit: Should we rename config to something like response here? Just to make it extra clear where it comes from.
There was a problem hiding this comment.
oh yes, that makes sense!
* master: (27 commits) ref(metric-meta): Add metric for total incoming metric meta (#2784) feat(server): Return global config status for downstream requests (#2765) ref(processor): Create event processor sub-module with related code (#2779) fix(metrics): Temporarily restore previous configuration keys for bucket splitting (#2780) feat(metrics): Add source context to code locations (#2781) ref(processor): Split off profile processor code into separate sub-module (#2778) ref(processor): Split off replay processing code into separate sub-module (#2776) ref(metrics): Partition and split metrics buckets just before sending (#2682) feat(spans): Allow well-known path segments in resource URLs (#2770) ref(processor): Move user and client reports processing into separate submodule (#2772) ref(crons): Include message_type in kafka message (#2723) ref(spans): Split tag mapping in specific configs (#2773) release: 23.11.2 feat(metric-meta): Normalize invalid metric names (#2769) feat(spans): Extract main_thread tag for spans (#2761) Add DE Deployments (#2746) ref(processor): Move sessions related code into separate sub-module (#2768) ref(metric-meta): Capture envelope payload in a sentry issue (#2767) release: 0.8.38 ref(normalization): Restore span processing to transactionprocessor (#2764) ...
follow-up to #2697
we now have a mechanism to delay envelope processing until a global config is available, however, we currently send back a default global config if one is requested from downstream, which kind of defeats the purpose if the downstream relay believes it has a valid global config but it doesn't.
To fix this, this pr will return an additional flag whether the config it sends is ready or not, up to date downstream relays will not use them but instead keep trying to get a ready global config and not process envelopes until they do. Older relays won't deserialize the status flag and their behaviour will be as before.