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.

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

Jump to bottom

Rate Limit indexed payloads only once #3662

Closed
Dav1dde opened this issue May 29, 2024 · 0 comments
Closed

Rate Limit indexed payloads only once #3662

Dav1dde opened this issue May 29, 2024 · 0 comments
Assignees
@Dav1dde

Comments

@Dav1dde
Copy link
Member

Dav1dde commented May 29, 2024 •
edited
Loading

Currently indexed payloads get rate limited twice for their non-indexed data category:

---> check quota without using it ----> extract metrics ----> store indexed
                                              \-------------> use quota -------> store metrics

We can simplify this to:

---> use quota ----> extract metrics ----> store indexed
                           \-------------> store metrics

This does require us to remember whether a metric was already rate limited through the indexed payload.

The upside here is, we save on individual Redis calls and it allows us to rate limit metrics (non-indexed) after aggregation reducing the total amount of rate limit checks (Redis calls) thanks to aggregation.

The new 'design':
PXL_20240529_122033553 MP~2
@Dav1dde Dav1dde self-assigned this May 29, 2024
This was referenced Jun 6, 2024
Closed
Merged
Dav1dde added a commit that referenced this issue Jun 18, 2024
@Dav1dde
ref(rate-limits): Rate limit envelopes instead of metrics for sampled…
d32f4aa 
…/indexed items (#3716)

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 mentioned this issue Jun 19, 2024
Merged
Dav1dde added a commit that referenced this issue Jun 24, 2024
@Dav1dde
ref(metrics): Aggregate metrics before rate limiting (#3746)
4ff3d83 
Implements the rest of #3662.
Fixes #3553.

Removes all pre-aggregation and rate limit steps before we receive a
project state, all rate limiting is done after aggregation.
@Dav1dde Dav1dde closed this as completed Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Dav1dde Dav1dde

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@Dav1dde