-
Notifications
You must be signed in to change notification settings - Fork 89
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
ref(project): Treat invalid project states as pending and unify state types #3770
Conversation
.enter( | ||
envelope, | ||
state.outcome_aggregator().clone(), | ||
state.test_store().clone(), | ||
group, | ||
) | ||
.map_err(BadStoreRequest::QueueFailed)?; | ||
envelope.scope(scoping); |
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.
Previously the split-out envelope was scoped by a call to check_envelope
in handle_processing
. That function takes a valid ProjectInfo
now, so we have to do it here.
#[serde(with = "LimitedProjectInfo")] | ||
#[serde(flatten)] | ||
info: ProjectInfo, | ||
} |
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.
I introduced a helper struct ParsedProjectState
to allow parsing the wire format in different places. Ideally this would be hidden away in the Serialize
/ Deserialize
implementation, but there is one location (load_local_states
) where we actually need to be able to parse a disabled state plus project keys.
) | ||
})?; | ||
self.dsc().map(|dsc| dsc.public_key) | ||
} |
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.
I needed a queue_key()
utility, so I decided to make sampling_key()
a method as well, instead of a standalone function.
@@ -74,7 +74,7 @@ use crate::service::ServiceError; | |||
use crate::services::global_config::GlobalConfigHandle; | |||
use crate::services::outcome::{DiscardReason, Outcome, TrackOutcome}; | |||
use crate::services::processor::event::FiltersStatus; | |||
use crate::services::project::ProjectState; | |||
use crate::services::project::ProjectInfo; |
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.
ProjectState
is now an enum. ProjectInfo
now contains most of the old fields of the old ProjectState
struct, minus disabled
.
@@ -279,7 +279,7 @@ mod tests { | |||
use crate::services::processor::{ | |||
ProcessEnvelope, ProcessingExtractedMetrics, ProcessingGroup, SpanGroup, | |||
}; | |||
use crate::services::project::ProjectState; | |||
use crate::services::project::ProjectInfo; |
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.
ProjectState
is now an enum. ProjectInfo
contains the fields that ProjectState
used to have (project_id
, config
, etc.), but is only accessible if the state is Enabled
.
// Unspooled envelopes need to be checked, just like we do on the fast path. | ||
if let Ok(CheckedEnvelope { envelope: Some(envelope), rate_limits: _ }) = metric!(timer(RelayTimers::ProjectCacheTaskDuration), task = "handle_check_envelope", { | ||
broker.handle_check_envelope(CheckEnvelope::new(managed_envelope)) | ||
}) { |
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.
This is new.
let ValidateEnvelope { envelope: context } = message; | ||
// TODO: check_envelope here? | ||
|
||
let ValidateEnvelope { |
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.
All the project checks are now done in handle_validate_envelope
. There is no more redundant check in handle_processing
.
states.insert(key.public_key, Arc::new(sanitized.clone())); | ||
let mut state = state.clone(); | ||
state.info.public_keys = smallvec::smallvec![key.clone()]; | ||
states.insert(key.public_key, ProjectState::from(state).sanitize()); |
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.
This makes it hard to make ParsedProjectState
a private concern of the (de)serializer: We need to peek into the config to get the project key.
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.
- Do we have an integration test for
invalid?
. - Don't forget to update the PR title.
- I think we should have a changelog entry for this, quite a big change.
/// - The upstream returned "pending" for this project (see [`crate::services::project_upstream`]). | ||
/// - The upstream returned an unparsable project so we have to try again. | ||
/// - The project has expired and must be treated as "has not been fetched". | ||
Pending, |
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.
Wonder if Missing
would be a better name, e.g. when a project is expired you return Pending
, but there is no gurantuee it's actually pending, it's just missing.
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.
I'm a bit hesitant to call it "missing" because when a projectconfig response does not contain a key, we treat the project as disabled and we previously called this state "missing", see
relay/relay-server/src/services/project.rs
Lines 122 to 127 in c9bd1f8
/// Project state for a missing project. | |
pub fn missing() -> Self { | |
ProjectState { | |
project_id: None, | |
last_change: None, | |
disabled: true, |
self.project_key | ||
); | ||
let is_enabled = match self.current_state() { | ||
ProjectState::Enabled(info) => info.has_feature(Feature::CustomMetrics), |
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.
I think it makes sense to implement has_feature
on the ProjectState
. This match
+ check
happens at least twice already.
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.
I like the idea, but how would you handle the Pending
variant? In this file alone we have two different behaviors (treat as enabled vs. early return).
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.
Fair point, I didn't look that deep, I assumed pending = false. If that is just becomes a footgun, better not do it.
} | ||
|
||
#[derive(Debug, Clone, Serialize, Deserialize)] | ||
pub struct ParsedProjectState { |
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.
Can we make this private and have the (de)-serialize impl of ProjectState
use this?
Possibly tricky with the limited versions though
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.
I had it hidden in a mod
that containted the (De)Serialize
implementations, but unfortunately, project_local
needs to look inside the config even for disabled states. See https://github.com/getsentry/relay/pull/3770/files/3452ab1d4af042327565421a8297cfa4afbb83e4#r1679462820
@@ -39,6 +39,15 @@ struct VersionQuery { | |||
version: u16, | |||
} | |||
|
|||
#[derive(Debug, Clone, Serialize)] | |||
#[serde(rename_all = "camelCase", remote = "ParsedProjectState")] | |||
struct LimitedParsedProjectState { |
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.
Should we move this to where ParsedProjectState
lives?
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.
It's only required for serialization in the endpoint, so it should in theory be private. But it makes sense to keep the Limited*
types next to their unlimited types to keep them in sync, so 👍.
/// This state is used for forwarding in Proxy mode. | ||
pub fn allowed() -> Self { | ||
Self::enabled(ProjectInfo { | ||
project_id: None, |
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.
Does it make sense to eliminate this in a follow-up, I think a ProjectInfo with a non optional project_id
and organization_id
could reduce some cruft and possibly also help solve our problem where feature flags in proxy mode are treated as disabled.
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.
Good idea, will note this as a follow-up.
} | ||
|
||
/// Sanitizes the contained project state. See [`ProjectState::sanitize`]. | ||
pub fn sanitize(self) -> Self { |
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.
pub fn sanitize(self) -> Self { | |
pub fn sanitized(self) -> Self { |
Seems more like a transition than a mutation
This reverts commit 96edc4e.
@Dav1dde there is
Any suggestions? Something like "Treat invalid project states as pending"? Or rather "ProjectState enum"? |
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.
Maybe something like: |
} | ||
} == data | ||
data = request_config(relay, packed, signature, version="3").json() | ||
assert data == {"configs": {}, "pending": [public_key]} |
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.
Relay was already configured to buffer envelopes for unparsable projects, but downstream relays would get them as disabled
and drop data anyway. This is now fixed.
This started buffering all envelopes to memory on S4S, need to investigate why. Reverts #3770.
… types (#3770) We currently deal with different envelope states inconsistently: Because the state of a project is encoded into many separate fields and types, it's hard to enforce that the config is validated correctly everywhere that it is used. For example, invalid (i.e. unparsable) project configs are sometimes treated the same as pending, sometimes the same as disabled.
We currently deal with different envelope states inconsistently: Because the state of a project is encoded into many separate fields and types, it's hard to enforce that the config is validated correctly everywhere that it is used.
For example, invalid (i.e. unparsable) project configs are sometimes treated the same as pending, sometimes the same as disabled:
relay/relay-server/src/services/project.rs
Lines 308 to 312 in b561ec3
relay/relay-server/src/services/project.rs
Lines 852 to 855 in b561ec3
New types
Future Work
GetOrFetch
byProjectState
.Pending
does not currently guarantee that another "fetch" is scheduled. We might be able to enforce this by typingPending
asPending(StateReceiver)
.ProjectState::Allowed
state to special case proxy relays.Fixes https://github.com/getsentry/team-ingest/issues/364.