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(rate-limits): Rate limit envelopes instead of metrics for sampled/indexed items #3716

Merged
merged 37 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
5a8f795
wip
Dav1dde Jun 6, 2024
4a044e5
wip
Dav1dde Jun 10, 2024
2ef6bc3
ref(rate-limits): Always enforce transactions on the indexed category
Dav1dde Jun 10, 2024
07f09ff
rework rate limiting of envelope
Dav1dde Jun 10, 2024
ae16d34
remove remainders of metrics extraction flag in rate limiting, fix te…
Dav1dde Jun 11, 2024
a005622
impl drop for MockRateLimiter
Dav1dde Jun 11, 2024
6bab4ea
cached limits, no indexed, actually expire limits
Dav1dde Jun 11, 2024
d82e336
basic rate limiting of metrics
Dav1dde Jun 11, 2024
650c0e6
is_indexed tag won't work for spans
Dav1dde Jun 11, 2024
dfaede4
metrics attempt
Dav1dde Jun 13, 2024
12e4244
dynamic sampling drops only indexed
Dav1dde Jun 13, 2024
c7f24de
fix conditional metric extraction
Dav1dde Jun 13, 2024
324693c
metrics snapshot tests, sampled
Dav1dde Jun 13, 2024
cac2374
profile outcomes test, python assert improved
Dav1dde Jun 13, 2024
856da6e
more integration tests
Dav1dde Jun 13, 2024
c6435a3
bucket key cost increased
Dav1dde Jun 13, 2024
7a8ac0c
insta snapshots, dbg, datetime assert fix, ds test oops
Dav1dde Jun 13, 2024
8b2ef82
Merge remote-tracking branch 'origin/master' into dav1d/rlb2
Dav1dde Jun 13, 2024
bf01bef
more insta snapshots
Dav1dde Jun 13, 2024
6046af9
fix rate limiting checking wrong limits, more tests
Dav1dde Jun 13, 2024
c45e8f6
more integration tests
Dav1dde Jun 13, 2024
e3a85bd
fix cached limits for no/all categories
Dav1dde Jun 13, 2024
cf2ec50
span dynamic sampling
Dav1dde Jun 13, 2024
9f2198d
more integration tests
Dav1dde Jun 13, 2024
2f09906
Merge branch 'master' into dav1d/rlb2
Dav1dde Jun 13, 2024
50f6b74
the final stretch
Dav1dde Jun 13, 2024
096bb52
minor cleanup
Dav1dde Jun 13, 2024
40a7f5a
cleanup metric extraction
Dav1dde Jun 14, 2024
1cf1ac3
changelog
Dav1dde Jun 14, 2024
79bb567
Merge remote-tracking branch 'origin/master' into dav1d/rlb2
Dav1dde Jun 17, 2024
ecb6e51
rename is_sampled to extracted_from_indexed
Dav1dde Jun 17, 2024
00b0bfd
unconditionally set extracted from indexed
Dav1dde Jun 17, 2024
8120f76
better documentation
Dav1dde Jun 17, 2024
148900f
fix consistent span transaction limit
Dav1dde Jun 17, 2024
a3d8db9
Update relay-metrics/src/bucket.rs
Dav1dde Jun 17, 2024
61bb79d
Update relay-quotas/src/rate_limit.rs
Dav1dde Jun 17, 2024
4442dfd
review changes
Dav1dde Jun 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
**Internal**:

- Treat arrays of pairs as key-value mappings during PII scrubbing. ([#3639](https://github.com/getsentry/relay/pull/3639))
- Rate limit envelopes instead of metrics for sampled/indexed items. ([#3716](https://github.com/getsentry/relay/pull/3716))
- Improve flush time calculation in metrics aggregator. ([#3726](https://github.com/getsentry/relay/pull/3726))

## 24.5.1
Expand Down
10 changes: 10 additions & 0 deletions relay-base-schema/src/data_category.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,19 @@ impl DataCategory {
pub fn index_category(self) -> Option<Self> {
match self {
Self::Transaction => Some(Self::TransactionIndexed),
Self::Span => Some(Self::SpanIndexed),
Self::Profile => Some(Self::ProfileIndexed),
_ => None,
}
}

/// Returns `true` if this data category is an indexed data category.
pub fn is_indexed(self) -> bool {
matches!(
self,
Self::TransactionIndexed | Self::SpanIndexed | Self::ProfileIndexed
)
}
}

impl fmt::Display for DataCategory {
Expand Down
18 changes: 17 additions & 1 deletion relay-metrics/src/aggregator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ struct BucketKey {
timestamp: UnixTimestamp,
metric_name: MetricName,
tags: BTreeMap<String, String>,
extracted_from_indexed: bool,
}

impl BucketKey {
Expand Down Expand Up @@ -725,6 +726,7 @@ impl Aggregator {
timestamp,
metric_name: bucket.name,
tags: bucket.tags,
extracted_from_indexed: bucket.metadata.extracted_from_indexed,
};
let key = validate_bucket_key(key, &self.config)?;

Expand Down Expand Up @@ -1012,6 +1014,7 @@ mod tests {
"c:transactions/foo@none",
),
tags: {},
extracted_from_indexed: false,
},
Counter(
85.0,
Expand Down Expand Up @@ -1061,10 +1064,11 @@ mod tests {
("hello".to_owned(), "world".to_owned()),
("answer".to_owned(), "42".to_owned()),
]),
extracted_from_indexed: false,
};
assert_eq!(
bucket_key.cost(),
80 + // BucketKey
88 + // BucketKey
5 + // name
(5 + 5 + 6 + 2) // tags
);
Expand Down Expand Up @@ -1107,6 +1111,7 @@ mod tests {
"c:transactions/foo@none",
),
tags: {},
extracted_from_indexed: false,
},
Counter(
84.0,
Expand All @@ -1120,6 +1125,7 @@ mod tests {
"c:transactions/foo@none",
),
tags: {},
extracted_from_indexed: false,
},
Counter(
42.0,
Expand Down Expand Up @@ -1242,6 +1248,7 @@ mod tests {
timestamp: UnixTimestamp::now(),
metric_name: "c:transactions/foo@none".into(),
tags: BTreeMap::new(),
extracted_from_indexed: false,
};
let fixed_cost = bucket_key.cost() + mem::size_of::<BucketValue>();
for (metric_name, metric_value, expected_added_cost) in [
Expand Down Expand Up @@ -1402,6 +1409,7 @@ mod tests {
tags.insert("another\0garbage".to_owned(), "bye".to_owned());
tags
},
extracted_from_indexed: false,
};

let mut bucket_key = validate_bucket_key(bucket_key, &test_config()).unwrap();
Expand All @@ -1427,6 +1435,7 @@ mod tests {
timestamp: UnixTimestamp::now(),
metric_name: "c:transactions/a_short_metric".into(),
tags: BTreeMap::new(),
extracted_from_indexed: false,
};
assert!(validate_bucket_key(short_metric, &test_config()).is_ok());

Expand All @@ -1435,6 +1444,7 @@ mod tests {
timestamp: UnixTimestamp::now(),
metric_name: "c:transactions/long_name_a_very_long_name_its_super_long_really_but_like_super_long_probably_the_longest_name_youve_seen_and_even_the_longest_name_ever_its_extremly_long_i_cant_tell_how_long_it_is_because_i_dont_have_that_many_fingers_thus_i_cant_count_the_many_characters_this_long_name_is".into(),
tags: BTreeMap::new(),
extracted_from_indexed: false,
};
let validation = validate_bucket_key(long_metric, &test_config());

Expand All @@ -1450,6 +1460,7 @@ mod tests {
timestamp: UnixTimestamp::now(),
metric_name: "c:transactions/a_short_metric_with_long_tag_key".into(),
tags: BTreeMap::from([("i_run_out_of_creativity_so_here_we_go_Lorem_Ipsum_is_simply_dummy_text_of_the_printing_and_typesetting_industry_Lorem_Ipsum_has_been_the_industrys_standard_dummy_text_ever_since_the_1500s_when_an_unknown_printer_took_a_galley_of_type_and_scrambled_it_to_make_a_type_specimen_book".into(), "tag_value".into())]),
extracted_from_indexed: false,
};
let validation = validate_bucket_key(short_metric_long_tag_key, &test_config()).unwrap();
assert_eq!(validation.tags.len(), 0);
Expand All @@ -1459,6 +1470,7 @@ mod tests {
timestamp: UnixTimestamp::now(),
metric_name: "c:transactions/a_short_metric_with_long_tag_value".into(),
tags: BTreeMap::from([("tag_key".into(), "i_run_out_of_creativity_so_here_we_go_Lorem_Ipsum_is_simply_dummy_text_of_the_printing_and_typesetting_industry_Lorem_Ipsum_has_been_the_industrys_standard_dummy_text_ever_since_the_1500s_when_an_unknown_printer_took_a_galley_of_type_and_scrambled_it_to_make_a_type_specimen_book".into())]),
extracted_from_indexed: false,
};
let validation = validate_bucket_key(short_metric_long_tag_value, &test_config()).unwrap();
assert_eq!(validation.tags.len(), 0);
Expand All @@ -1476,6 +1488,7 @@ mod tests {
timestamp: UnixTimestamp::now(),
metric_name: "c:transactions/a_short_metric".into(),
tags: BTreeMap::from([("foo".into(), tag_value.clone())]),
extracted_from_indexed: false,
};
let validated_bucket = validate_metric_tags(short_metric, &test_config());
assert_eq!(validated_bucket.tags["foo"], tag_value);
Expand Down Expand Up @@ -1583,6 +1596,7 @@ mod tests {
received_at: Some(
UnixTimestamp(999994711),
),
extracted_from_indexed: false,
},
]
"###);
Expand All @@ -1609,6 +1623,7 @@ mod tests {
timestamp,
metric_name: "c:transactions/foo".into(),
tags: BTreeMap::new(),
extracted_from_indexed: false,
};

// Second bucket has a timestamp in this hour.
Expand All @@ -1618,6 +1633,7 @@ mod tests {
timestamp,
metric_name: "c:transactions/foo".into(),
tags: BTreeMap::new(),
extracted_from_indexed: false,
};

let flush_time_1 = get_flush_time(&config, reference_time, &bucket_key_1);
Expand Down
30 changes: 27 additions & 3 deletions relay-metrics/src/bucket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -759,6 +759,17 @@ pub struct BucketMetadata {
/// This field should be set to the time in which the first metric of a specific bucket was
/// received in the outermost internal Relay.
pub received_at: Option<UnixTimestamp>,

/// Is `true` if this metric was extracted from a sampled/indexed envelope item.
///
/// The final dyanmic sampling decision is always made in processing Relays,
/// if a metric was extracted from an item which is sampled, this flag is `true`.
Dav1dde marked this conversation as resolved.
Show resolved Hide resolved
///
/// Since these metrics from samples carry additional information, e.g. they don't
/// require rate limiting since the sample they've been extracted from was already
/// rate limited, this flag must be included in the aggregation key when aggregation buckets.
#[serde(skip)]
pub extracted_from_indexed: bool,
}

impl BucketMetadata {
Expand All @@ -769,6 +780,7 @@ impl BucketMetadata {
Self {
merges: 1,
received_at: Some(received_at),
extracted_from_indexed: false,
}
}

Expand All @@ -793,6 +805,7 @@ impl Default for BucketMetadata {
Self {
merges: 1,
received_at: None,
extracted_from_indexed: false,
}
}
}
Expand Down Expand Up @@ -927,6 +940,7 @@ mod tests {
metadata: BucketMetadata {
merges: 1,
received_at: None,
extracted_from_indexed: false,
},
}
"###);
Expand Down Expand Up @@ -961,6 +975,7 @@ mod tests {
metadata: BucketMetadata {
merges: 1,
received_at: None,
extracted_from_indexed: false,
},
}
"###);
Expand Down Expand Up @@ -1013,6 +1028,7 @@ mod tests {
metadata: BucketMetadata {
merges: 1,
received_at: None,
extracted_from_indexed: false,
},
}
"###);
Expand Down Expand Up @@ -1073,6 +1089,7 @@ mod tests {
metadata: BucketMetadata {
merges: 1,
received_at: None,
extracted_from_indexed: false,
},
}
"###);
Expand Down Expand Up @@ -1103,6 +1120,7 @@ mod tests {
metadata: BucketMetadata {
merges: 1,
received_at: None,
extracted_from_indexed: false,
},
}
"###);
Expand All @@ -1127,6 +1145,7 @@ mod tests {
metadata: BucketMetadata {
merges: 1,
received_at: None,
extracted_from_indexed: false,
},
}
"###);
Expand Down Expand Up @@ -1330,6 +1349,7 @@ mod tests {
received_at: Some(
UnixTimestamp(1615889440),
),
extracted_from_indexed: false,
},
},
]
Expand Down Expand Up @@ -1371,6 +1391,7 @@ mod tests {
received_at: Some(
UnixTimestamp(1615889440),
),
extracted_from_indexed: false,
},
},
]
Expand Down Expand Up @@ -1459,7 +1480,8 @@ mod tests {
metadata,
BucketMetadata {
merges: 2,
received_at: None
received_at: None,
extracted_from_indexed: false,
}
);

Expand All @@ -1469,7 +1491,8 @@ mod tests {
metadata,
BucketMetadata {
merges: 3,
received_at: Some(UnixTimestamp::from_secs(10))
received_at: Some(UnixTimestamp::from_secs(10)),
extracted_from_indexed: false,
}
);

Expand All @@ -1479,7 +1502,8 @@ mod tests {
metadata,
BucketMetadata {
merges: 4,
received_at: Some(UnixTimestamp::from_secs(10))
received_at: Some(UnixTimestamp::from_secs(10)),
extracted_from_indexed: false,
}
);
}
Expand Down
2 changes: 1 addition & 1 deletion relay-metrics/src/view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ impl<'a> BucketView<'a> {

BucketMetadata {
merges,
received_at: self.inner.metadata.received_at,
..self.inner.metadata
}
}

Expand Down
Loading
Loading