Skip to content
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

Merged
merged 52 commits into from
Jul 18, 2024
Merged
Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
dc191af
state -> info
jjbayer Jun 18, 2024
23819cd
Split into ProjectFetchState and ProjectInfo
jjbayer Jun 18, 2024
6354f02
wip
jjbayer Jun 19, 2024
b1b551e
wip
jjbayer Jun 19, 2024
9eb1a3c
Merge remote-tracking branch 'origin/master' into ref/server-state-cl…
jjbayer Jun 24, 2024
c75f596
fix: limited config
jjbayer Jun 24, 2024
2fe1ef8
wip
jjbayer Jun 24, 2024
d8e4661
wip
jjbayer Jun 25, 2024
d97864e
wip
jjbayer Jun 25, 2024
f54cc25
wip
jjbayer Jun 25, 2024
f144966
ref: check_envelope
jjbayer Jun 27, 2024
c26ab17
fix
jjbayer Jul 2, 2024
41d5305
cargo check passes
jjbayer Jul 2, 2024
6aadc85
it compiles
jjbayer Jul 2, 2024
6e8730a
Merge remote-tracking branch 'origin/master' into ref/server-state-cl…
jjbayer Jul 2, 2024
0f5a299
fix: lint
jjbayer Jul 2, 2024
ad97f80
fix: Initialize project with expired state.
jjbayer Jul 2, 2024
0fefe75
Merge remote-tracking branch 'origin/master' into ref/server-state-cl…
jjbayer Jul 5, 2024
72321f8
get rid of non_expired_state
jjbayer Jul 10, 2024
d9cb3dc
simplify
jjbayer Jul 10, 2024
da5a98e
rm unused function
jjbayer Jul 10, 2024
ab3384d
lint
jjbayer Jul 10, 2024
8218dfc
fix: expiry
jjbayer Jul 11, 2024
5abb665
fix: Parse disabled
jjbayer Jul 11, 2024
90dffe2
ref: rm handle_processing
jjbayer Jul 11, 2024
946939b
fix: Outcomes for disabled state
jjbayer Jul 11, 2024
80f9ef8
Merge remote-tracking branch 'origin/master' into ref/server-state-cl…
jjbayer Jul 11, 2024
97f67c1
fix: Check envelope
jjbayer Jul 11, 2024
a9622ea
fix: Keep scope when splitting envelope
jjbayer Jul 12, 2024
2ba57c5
ref: metric_meta flush
jjbayer Jul 12, 2024
4e54787
Merge remote-tracking branch 'origin/master' into ref/server-state-cl…
jjbayer Jul 12, 2024
2d11c17
fix
jjbayer Jul 12, 2024
41ac196
fix: Retry invalid
jjbayer Jul 12, 2024
39c29ce
fix: Reject envelope
jjbayer Jul 12, 2024
e035308
Merge remote-tracking branch 'origin/master' into ref/server-state-cl…
jjbayer Jul 16, 2024
0d89b86
fix
jjbayer Jul 16, 2024
b23ae22
fix: metric meta
jjbayer Jul 16, 2024
fb58fcd
doc
jjbayer Jul 16, 2024
76bceb7
file structure
jjbayer Jul 16, 2024
8cbf021
doc
jjbayer Jul 16, 2024
5c50153
lint
jjbayer Jul 16, 2024
3452ab1
Little things
jjbayer Jul 16, 2024
96edc4e
feat: Use existing backoff loop for pending
jjbayer Jul 17, 2024
c49469d
Revert "feat: Use existing backoff loop for pending"
jjbayer Jul 17, 2024
018e356
Use backoff loop for invalid
jjbayer Jul 17, 2024
c3ac129
doc
jjbayer Jul 17, 2024
e5e0b8a
Merge remote-tracking branch 'origin/master' into ref/server-state-cl…
jjbayer Jul 17, 2024
d5e984d
changelog
jjbayer Jul 17, 2024
4baf239
ref: QueueKey
jjbayer Jul 17, 2024
f192372
ref: Move limited
jjbayer Jul 18, 2024
1510b07
ref: review comments
jjbayer Jul 18, 2024
8ffb253
lint: doc
jjbayer Jul 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion relay-server/src/endpoints/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,16 +297,18 @@ fn queue_envelope(
}

// Split off the envelopes by item type.
let scoping = managed_envelope.scoping();
let envelopes = ProcessingGroup::split_envelope(*managed_envelope.take_envelope());
for (group, envelope) in envelopes {
let envelope = buffer_guard
let mut envelope = buffer_guard
.enter(
envelope,
state.outcome_aggregator().clone(),
state.test_store().clone(),
group,
)
.map_err(BadStoreRequest::QueueFailed)?;
envelope.scope(scoping);
Copy link
Member Author

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.

state.project_cache().send(ValidateEnvelope::new(envelope));
}
// The entire envelope is taken for a split above, and it's empty at this point, we can just
Expand Down
45 changes: 33 additions & 12 deletions relay-server/src/endpoints/project_configs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::endpoints::forward;
use crate::extractors::SignedJson;
use crate::service::ServiceState;
use crate::services::global_config::{self, StatusResponse};
use crate::services::project::{LimitedProjectState, ProjectState};
use crate::services::project::{LimitedProjectInfo, ParsedProjectState, ProjectInfo, ProjectState};
use crate::services::project_cache::{GetCachedProjectState, GetProjectState};

/// V2 version of this endpoint.
Expand All @@ -39,6 +39,15 @@ struct VersionQuery {
version: u16,
}

#[derive(Debug, Clone, Serialize)]
#[serde(rename_all = "camelCase", remote = "ParsedProjectState")]
struct LimitedParsedProjectState {
Copy link
Member

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?

Copy link
Member Author

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 👍.

disabled: bool,
Dav1dde marked this conversation as resolved.
Show resolved Hide resolved
#[serde(with = "LimitedProjectInfo")]
#[serde(flatten)]
info: ProjectInfo,
}
Copy link
Member Author

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.


/// The type returned for each requested project config.
///
/// Wrapper on top the project state which encapsulates information about how ProjectState
Expand All @@ -49,13 +58,13 @@ struct VersionQuery {
#[derive(Debug, Clone, Serialize)]
#[serde(untagged)]
enum ProjectStateWrapper {
Full(ProjectState),
Limited(#[serde(with = "LimitedProjectState")] ProjectState),
Full(ParsedProjectState),
Limited(#[serde(with = "LimitedParsedProjectState")] ParsedProjectState),
}

impl ProjectStateWrapper {
/// Create a wrapper which forces serialization into external or internal format
pub fn new(state: ProjectState, full: bool) -> Self {
pub fn new(state: ParsedProjectState, full: bool) -> Self {
if full {
Self::Full(state)
} else {
Expand All @@ -76,7 +85,7 @@ impl ProjectStateWrapper {
#[derive(Debug, Serialize)]
#[serde(rename_all = "camelCase")]
struct GetProjectStatesResponseWrapper {
configs: HashMap<ProjectKey, Option<ProjectStateWrapper>>,
configs: HashMap<ProjectKey, ProjectStateWrapper>,
#[serde(skip_serializing_if = "Vec::is_empty")]
pending: Vec<ProjectKey>,
#[serde(skip_serializing_if = "Option::is_none")]
Expand Down Expand Up @@ -123,7 +132,6 @@ async fn inner(
project_cache
.send(GetProjectState::new(project_key).no_cache(no_cache))
.await
.map(Some)
};

(project_key, state_result)
Expand All @@ -146,23 +154,36 @@ async fn inner(
let mut pending = Vec::with_capacity(keys_len);
let mut configs = HashMap::with_capacity(keys_len);
for (project_key, state_result) in future::join_all(futures).await {
let Some(project_state) = state_result? else {
pending.push(project_key);
continue;
let project_info = match state_result? {
ProjectState::Enabled(info) => info,
ProjectState::Disabled => {
// Don't insert project config. Downstream Relay will consider it disabled.
continue;
}
ProjectState::Pending => {
pending.push(project_key);
continue;
}
};

// If public key is known (even if rate-limited, which is Some(false)), it has
// access to the project config
let has_access = relay.internal
|| project_state
|| project_info
.config
.trusted_relays
.contains(&relay.public_key);

if has_access {
let full = relay.internal && inner.full_config;
let wrapper = ProjectStateWrapper::new((*project_state).clone(), full);
configs.insert(project_key, Some(wrapper));
let wrapper = ProjectStateWrapper::new(
ParsedProjectState {
disabled: false,
info: project_info.as_ref().clone(),
},
full,
);
configs.insert(project_key, wrapper);
} else {
relay_log::debug!(
relay = %relay.public_key,
Expand Down
27 changes: 27 additions & 0 deletions relay-server/src/envelope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
//!
//! ```

use relay_base_schema::project::ProjectKey;
use std::borrow::Borrow;
use std::collections::BTreeMap;
use std::fmt;
Expand All @@ -52,6 +53,7 @@ use smallvec::SmallVec;

use crate::constants::DEFAULT_EVENT_RETENTION;
use crate::extractors::{PartialMeta, RequestMeta};
use crate::services::spooler::QueueKey;

pub const CONTENT_TYPE: &str = "application/x-sentry-envelope";

Expand Down Expand Up @@ -1218,6 +1220,31 @@ impl Envelope {
self.headers.sent_at
}

/// Returns the project key defined in the `trace` header of the envelope.
///
/// This function returns `None` if:
/// - there is no [`DynamicSamplingContext`] in the envelope headers.
/// - there are no transactions or events in the envelope, since in this case sampling by trace is redundant.
pub fn sampling_key(&self) -> Option<ProjectKey> {
// If the envelope item is not of type transaction or event, we will not return a sampling key
// because it doesn't make sense to load the root project state if we don't perform trace
// sampling.
self.get_item_by(|item| {
matches!(
item.ty(),
ItemType::Transaction | ItemType::Event | ItemType::Span
)
})?;
self.dsc().map(|dsc| dsc.public_key)
}
Copy link
Member Author

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.


pub fn queue_key(&self) -> QueueKey {
jjbayer marked this conversation as resolved.
Show resolved Hide resolved
QueueKey {
own_key: self.meta().public_key(),
sampling_key: self.sampling_key().unwrap_or(self.meta().public_key()),
}
}

/// Sets the event id on the envelope.
pub fn set_event_id(&mut self, event_id: EventId) {
self.headers.event_id = Some(event_id);
Expand Down
Loading
Loading