Skip to content

ref(profile-chunks): Finally forbid profile chunk items without platform header #5507

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.

Sign up for GitHub

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

Open
Dav1dde wants to merge 1 commit into
base: dav1d/profile-chunks-processing
Choose a base branch
from
+36 −78
Open

ref(profile-chunks): Finally forbid profile chunk items without platform header #5507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
**Features**:

- Double write to legacy attributes for backwards compatibility. ([#5490](https://github.com/getsentry/relay/pull/5490))
- Forbid invalid profile chunks without a platform header. ([#5507](https://github.com/getsentry/relay/pull/5507))

**Internal**:

Expand Down
5 changes: 3 additions & 2 deletions relay-server/src/envelope/item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,12 @@ impl Item {
(DataCategory::Span, item_count),
(DataCategory::SpanIndexed, item_count),
],
// NOTE: semantically wrong, but too expensive to parse.
ItemType::ProfileChunk => match self.profile_type() {
Some(ProfileType::Backend) => smallvec![(DataCategory::ProfileChunk, item_count)],
Some(ProfileType::Ui) => smallvec![(DataCategory::ProfileChunkUi, item_count)],
None => smallvec![],
// Profile chunks without platform/profile type are considered invalid,
// fallback to `profile chunk` to still get outcomes.
None => smallvec![(DataCategory::ProfileChunk, item_count)],
},
ItemType::Integration => match self.integration() {
Some(Integration::Logs(LogsIntegration::OtelV1 { .. })) => smallvec![
Expand Down
4 changes: 3 additions & 1 deletion relay-server/src/processing/profile_chunks/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,9 @@ impl Counted for SerializedProfileChunks {
match pc.profile_type() {
Some(ProfileType::Ui) => ui += 1,
Some(ProfileType::Backend) => backend += 1,
None => {}
// These are invalid and will be dropped, but we default outcomes to the backend
// profile chunk category.
None => backend += 1,
}
}

Expand Down
34 changes: 6 additions & 28 deletions relay-server/src/processing/profile_chunks/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
use relay_profiling::ProfileType;
use relay_quotas::DataCategory;

use crate::envelope::{ContentType, Item, ItemType};
use crate::processing::Context;
use crate::processing::Managed;
Expand All @@ -18,40 +15,21 @@ pub fn process(profile_chunks: &mut Managed<SerializedProfileChunks>, ctx: Conte

profile_chunks.retain(
|pc| &mut pc.profile_chunks,
|item, records| -> Result<()> {
|item, _| -> Result<()> {
let pc = relay_profiling::ProfileChunk::new(item.payload())?;

// Validate the item inferred profile type with the one from the payload,
// or if missing set it.
//
// This is currently necessary to ensure profile chunks are emitted in the correct
// data category, as well as rate limited with the correct data category.
//
// In the future we plan to make the profile type on the item header a necessity.
// For more context see also: <https://github.com/getsentry/relay/pull/4595>.
if item
.profile_type()
.is_some_and(|pt| pt != pc.profile_type())
{
// Profile chunks must carry the same profile type in the item header as well as the
// payload.
if item.profile_type().is_none_or(|pt| pt != pc.profile_type()) {
relay_log::debug!("dropping profile chunk due to profile type/platform mismatch");
return Err(relay_profiling::ProfileError::InvalidProfileType.into());
}

// Update the profile type to ensure the following outcomes are emitted in the correct
// data category.
//
// Once the item header on the item is required, this is no longer required.
if item.profile_type().is_none() {
item.set_profile_type(pc.profile_type());
match pc.profile_type() {
ProfileType::Ui => records.modify_by(DataCategory::ProfileChunkUi, 1),
ProfileType::Backend => records.modify_by(DataCategory::ProfileChunk, 1),
}
}

pc.filter(client_ip, filter_settings, ctx.global_config)?;

let expanded = pc.expand()?;
if expanded.len() > ctx.config.max_profile_size() {
relay_log::debug!("dropping profile chunk exceeding the size limit");
return Err(relay_profiling::ProfileError::ExceedSizeLimit.into());
}

Expand Down
34 changes: 7 additions & 27 deletions relay-server/src/utils/rate_limits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,9 +643,8 @@ impl Enforcement {
}
ItemType::Span => !self.spans_indexed.is_active(),
ItemType::ProfileChunk => match item.profile_type() {
Some(ProfileType::Backend) => !self.profile_chunks.is_active(),
Some(ProfileType::Backend) | None => !self.profile_chunks.is_active(),
Some(ProfileType::Ui) => !self.profile_chunks_ui.is_active(),
None => true,
},
ItemType::TraceMetric => !self.trace_metrics.is_active(),
ItemType::Integration => match item.integration() {
Expand Down Expand Up @@ -1618,28 +1617,9 @@ mod tests {
}

/// Limit profile chunks.
#[tokio::test]
async fn test_enforce_limit_profile_chunks_no_profile_type() {
// In this test we have profile chunks which have not yet been classified, which means they
// should not be rate limited.
let mut envelope = envelope![ProfileChunk, ProfileChunk];

let mock = mock_limiter(&[DataCategory::ProfileChunk]);
let (enforcement, limits) = enforce_and_apply(mock.clone(), &mut envelope, None).await;
assert!(!limits.is_limited());
assert_eq!(get_outcomes(enforcement), vec![]);

let mock = mock_limiter(&[DataCategory::ProfileChunkUi]);
let (enforcement, limits) = enforce_and_apply(mock.clone(), &mut envelope, None).await;
assert!(!limits.is_limited());
assert_eq!(get_outcomes(enforcement), vec![]);

assert_eq!(envelope.envelope().len(), 2);
}

#[tokio::test]
async fn test_enforce_limit_profile_chunks_ui() {
let mut envelope = envelope![];
let mut envelope = envelope![ProfileChunk];

let mut item = Item::new(ItemType::ProfileChunk);
item.set_profile_type(ProfileType::Backend);
Expand All @@ -1652,11 +1632,11 @@ mod tests {
let (enforcement, limits) = enforce_and_apply(mock.clone(), &mut envelope, None).await;

assert!(limits.is_limited());
assert_eq!(envelope.envelope().len(), 1);
assert_eq!(envelope.envelope().len(), 2);
mock.lock()
.await
.assert_call(DataCategory::ProfileChunkUi, 1);
mock.lock().await.assert_call(DataCategory::ProfileChunk, 1);
mock.lock().await.assert_call(DataCategory::ProfileChunk, 2);

assert_eq!(
get_outcomes(enforcement),
Expand All @@ -1666,7 +1646,7 @@ mod tests {

#[tokio::test]
async fn test_enforce_limit_profile_chunks_backend() {
let mut envelope = envelope![];
let mut envelope = envelope![ProfileChunk];

let mut item = Item::new(ItemType::ProfileChunk);
item.set_profile_type(ProfileType::Backend);
Expand All @@ -1683,11 +1663,11 @@ mod tests {
mock.lock()
.await
.assert_call(DataCategory::ProfileChunkUi, 1);
mock.lock().await.assert_call(DataCategory::ProfileChunk, 1);
mock.lock().await.assert_call(DataCategory::ProfileChunk, 2);

assert_eq!(
get_outcomes(enforcement),
vec![(DataCategory::ProfileChunk, 1)]
vec![(DataCategory::ProfileChunk, 2)]
);
}

Expand Down
36 changes: 16 additions & 20 deletions tests/integration/test_profile_chunks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
}


def sample_profile_v2_envelope(platform=None):
def sample_profile_v2_envelope(platform="cocoa"):
envelope = Envelope()

with open(
Expand All @@ -47,7 +47,7 @@ def sample_profile_v2_envelope(platform=None):
return envelope


def android_profile_chunk_envelope(platform=None):
def android_profile_chunk_envelope(platform="android"):
envelope = Envelope()

with open(
Expand Down Expand Up @@ -155,7 +155,13 @@ def test_profile_chunk_outcomes_invalid(
"chunk_id": "11111111111111111111111111111111",
"platform": "thisisnotvalid",
}
envelope.add_item(Item(payload=PayloadRef(json=payload), type="profile_chunk"))
envelope.add_item(
Item(
payload=PayloadRef(json=payload),
type="profile_chunk",
headers={"platform": "thisisnotvalid"},
)
)

upstream.send_envelope(project_id, envelope)

Expand All @@ -178,7 +184,6 @@ def test_profile_chunk_outcomes_invalid(
profiles_consumer.assert_empty()


@pytest.mark.parametrize("item_header_platform", [None, "cocoa"])
@pytest.mark.parametrize(
["envelope"],
[
Expand All @@ -195,7 +200,6 @@ def test_profile_chunk_outcomes_rate_limited(
outcomes_consumer,
profiles_consumer,
envelope,
item_header_platform,
):
"""
Tests that Relay reports correct outcomes when profile chunks are rate limited.
Expand Down Expand Up @@ -227,7 +231,7 @@ def test_profile_chunk_outcomes_rate_limited(
]

# Create and send envelope containing the profile chunk
envelope = envelope(item_header_platform)
envelope = envelope()
upstream = relay_with_processing(TEST_CONFIG)
upstream.send_envelope(project_id, envelope)

Expand Down Expand Up @@ -257,7 +261,7 @@ def test_profile_chunk_outcomes_rate_limited(
[
("cocoa", "profile_chunk_ui"),
("node", "profile_chunk"),
(None, "profile_chunk"), # Special case, currently this will forward
(None, "profile_chunk"),
],
)
@pytest.mark.parametrize(
Expand All @@ -280,10 +284,6 @@ def test_profile_chunk_outcomes_rate_limited_fast(
"""
Tests that Relay reports correct outcomes when profile chunks are rate limited already in the
fast-path, using the item header.

The test is parameterized to also *not* send the necessary item header, in which case this currently
asserts the chunk is let through. Once Relay's behaviour is changed to reject or profile chunks
without the necessary headers or the profile type is defaulted this test needs to be adjusted accordingly.
"""
project_id = 42
project_config = mini_sentry.add_full_project_config(project_id)["config"]
Expand All @@ -305,15 +305,11 @@ def test_profile_chunk_outcomes_rate_limited_fast(
upstream = relay(mini_sentry)
upstream.send_envelope(project_id, envelope)

if platform is None:
envelope = mini_sentry.get_captured_envelope()
assert [item.type for item in envelope.items] == ["profile_chunk"]
else:
outcome = mini_sentry.get_client_report()
assert outcome["rate_limited_events"] == [
{"category": category, "quantity": 1, "reason": "profile_chunks_exceeded"}
]
assert mini_sentry.captured_envelopes.empty()
outcome = mini_sentry.get_client_report()
assert outcome["rate_limited_events"] == [
{"category": category, "quantity": 1, "reason": "profile_chunks_exceeded"}
]
assert mini_sentry.captured_envelopes.empty()


@pytest.mark.parametrize(
Expand Down
Loading