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

Conversation

Dav1dde
Copy link
Member

@Dav1dde Dav1dde commented Jun 10, 2024

Partially Implements: #3662 (does not move rate limits to after aggregation).

Changes:

  • Data in an envelope is always considered to be both Indexed and Non-Indexed, for example: Until a transaction is removed from an envelope it is considered to be both Transaction and TransactionIndexed. Only dynamic sampling, when dropping the transaction from the envelope, emits one outcome with the TransactionIndexed category.
  • A few tests asserted the old/wrong behaviour of only emitting a single outcome, e.g. when an inbound filter filters a transaction we used to only get a Transaction outcome but not a TransactionIndexed, these tests have been aligned.
  • When checking rate limits for an envelope, quota on the 'base' category (e.g. Transaction) is now consumed.
  • Metrics now remember whether they were extracted from a sampled envelope item.
  • Metrics are immediately rate limited using the same rate limits as the ones used for the envelope.
  • Metrics from sampled envelopes will not be rate limited again.
  • Improved cached rate limits with a new type struct, which surfaced a bug that they weren't always correctly expired (fixed).

Future Improvements:

  • Move metrics rate limiting to after aggregation
  • Stream line metrics rate limiting from 4 (cached transactions/spans, transactions/spans, cached metric_buckets, metric_bucket) different checks to 2 (cached, non-cached)

@Dav1dde Dav1dde self-assigned this Jun 11, 2024
@Dav1dde Dav1dde marked this pull request as ready for review June 14, 2024 07:27
@Dav1dde Dav1dde requested a review from a team as a code owner June 14, 2024 07:27
@Dav1dde Dav1dde changed the title ref(rate-limits): Rate limit envelopes instead of transactions for sampled/indexed items ref(rate-limits): Rate limit envelopes instead of metrics for sampled/indexed items Jun 17, 2024
relay-metrics/src/bucket.rs Outdated Show resolved Hide resolved
relay-quotas/src/rate_limit.rs Outdated Show resolved Hide resolved
relay-quotas/src/rate_limit.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
relay-server/src/metrics/rate_limits.rs Outdated Show resolved Hide resolved
relay-server/src/services/processor.rs Outdated Show resolved Hide resolved
relay-server/src/services/processor/dynamic_sampling.rs Outdated Show resolved Hide resolved
tests/integration/test_metrics.py Outdated Show resolved Hide resolved
relay-server/src/utils/rate_limits.rs Outdated Show resolved Hide resolved
@Dav1dde Dav1dde enabled auto-merge (squash) June 18, 2024 06:12
@Dav1dde Dav1dde merged commit d32f4aa into master Jun 18, 2024
22 checks passed
@Dav1dde Dav1dde deleted the dav1d/rlb2 branch June 18, 2024 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants