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

feat(cardinality): Implement Redis set based cardinality limiter #2745

Merged
merged 22 commits into from
Dec 12, 2023

Conversation

Dav1dde
Copy link
Member

@Dav1dde Dav1dde commented Nov 20, 2023

Introduces a new relay-cardinality module, initially it is only housing the cardinality limiter, but at some point there may be more functionality related to cardinality (e.g. smart detection of high cardinality tags).

Implements a very basic cardinality limiter using one Redis set per organization and namespace to track cardinality.

API is still very much work in progress, this works for now but will definitely need to be changed in the future when the requirements for the cardinality limiter change. Outcomes, Tag erasure etc.

Epic: #2717

@Dav1dde Dav1dde force-pushed the feat/cardinality-limiter branch from 4ebfe3c to 1935241 Compare December 5, 2023 16:10
@Dav1dde Dav1dde force-pushed the feat/cardinality-limiter branch from 1935241 to ec47e21 Compare December 5, 2023 16:14
@Dav1dde Dav1dde changed the title feat(cardinality): Introduce new cardinality module feat(cardinality): Implement Redis set based cardinality limiter Dec 6, 2023
@Dav1dde Dav1dde force-pushed the feat/cardinality-limiter branch from b5b77f1 to 8bc9bdb Compare December 7, 2023 09:10
flush_partitions: Option<u64>,
) -> BTreeMap<Option<u64>, Vec<Bucket>> {
let flush_partitions = match flush_partitions {
None => return BTreeMap::from([(None, buckets)]),
None => return BTreeMap::from([(None, buckets.into_iter().collect())]),
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 checked with godbolt this is optimized away when passing a vector already

@Dav1dde Dav1dde marked this pull request as ready for review December 7, 2023 11:05
@Dav1dde Dav1dde requested a review from a team as a code owner December 7, 2023 11:05
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the look of this! Just a bunch of random comments for now, will do a more in-depth review later.

relay-cardinality/src/limiter.rs Outdated Show resolved Hide resolved
relay-cardinality/src/limiter.rs Outdated Show resolved Hide resolved
relay-cardinality/src/limiter.rs Outdated Show resolved Hide resolved
relay-cardinality/Cargo.toml Outdated Show resolved Hide resolved
relay-cardinality/src/redis.rs Show resolved Hide resolved
relay-cardinality/src/redis.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
relay-cardinality/Cargo.toml Outdated Show resolved Hide resolved
relay-cardinality/src/limiter.rs Outdated Show resolved Hide resolved
relay-cardinality/src/limiter.rs Show resolved Hide resolved
relay-cardinality/src/limiter.rs Outdated Show resolved Hide resolved
relay-cardinality/src/redis.rs Outdated Show resolved Hide resolved
relay-cardinality/src/redis.rs Show resolved Hide resolved
Comment on lines +1290 to +1292
if !enable_cardinality_limiter {
return Ok(Box::new(buckets.into_iter()));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Why not check the flag outside of this function? Seems more straight-forward than passing a boolean that makes the function behave like a noop (but this might just be my personal preference).

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 implemented the function pretty much only for this check, so I can early return, otherwise it becomes very complex with the cfg(feature = "processing")

fn check_cardinality_limits(
&self,
enable_cardinality_limiter: bool,
_organization_id: u64,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should make this function #[cfg(feature = "processing")]?

Suggested change
_organization_id: u64,
organization_id: u64,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is needed for non-processing, the entire function just exists to handle the if case with an early return to make the code somewhat readable and not a pure if else cfg mess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it still a mess with the if_processing! macro?

relay-cardinality/src/redis.rs Outdated Show resolved Hide resolved
@Dav1dde Dav1dde force-pushed the feat/cardinality-limiter branch from 90e2a3e to 9121915 Compare December 11, 2023 14:21
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ship it! Before opting in all orgs we should transition from copying entire redis sets to a Lua script, as you suggested.

@@ -1,4 +1,4 @@
//! Metrics Cardinality Limiter
//! Relay Cardinality Module
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily for this PR, but since this is a new crate, we should take a look at what make doc-rust produces and whether we're happy with the documentation. Other crates have a summary of what they do in the crate-level docs, e.g.

//! Metric protocol, aggregation and processing for Sentry.
//!
//! Metrics are high-volume values sent from Sentry clients, integrations, or extracted from errors
//! and transactions, that can be aggregated and queried over large time windows. As opposed to rich
//! errors and transactions, metrics carry relatively little context information in tags with low
//! cardinality.
//!
//! # Protocol
//!
//! Clients submit metrics in a [text-based protocol](Bucket) based on StatsD. See the [field
//! documentation](Bucket#fields) on `Bucket` for more information on the components. A sample
//! submission looks like this:
//!
//! ```text
#![doc = include_str!("../tests/fixtures/buckets.statsd.txt")]
//! ```
//!
//! The metric type is part of its signature just like the unit. Therefore, it is allowed to reuse a
//! metric name for multiple metric types, which will result in multiple metrics being recorded.
//!
//! # Metric Envelopes
//!
//! To send one or more metrics to Relay, the raw protocol is enclosed in an envelope item of type
//! `metrics`:
//!
//! ```text
//! {}
//! {"type": "statsd", ...}
#![doc = include_str!("../tests/fixtures/buckets.statsd.txt")]
//! ...
//! ```
//!
//! Note that the name format used in the statsd protocol is different from the MRI: Metric names
//! are not prefixed with `<ty>:` as the type is somewhere else in the protocol. If no metric
//! namespace is specified, the `"custom"` namespace is assumed.
//!
//! Optionally, a timestamp can be added to every line of the submitted envelope. The timestamp has
//! to be a valid Unix timestamp (UTC) and must be prefixed with `T`. If it is omitted, the
//! `received` time of the envelope is assumed.
//!
//! # Aggregation
//!
//! Relay accumulates all metrics in [time buckets](Bucket) before sending them onwards. Aggregation
//! is handled by the [`Aggregator`], which should be created once for the entire system. It flushes
//! aggregates in regular intervals, either shortly after their original time window has passed or
//! with a debounce delay for backdated submissions.
//!
//! **Warning**: With chained Relays submission delays accumulate.
//!
//! Aggregate buckets are encoded in JSON with the following schema:
//!
//! ```json
#![doc = include_str!("../tests/fixtures/buckets.json")]
//! ```
//!
//! # Ingestion
//!
//! Processing Relays write aggregate buckets into the ingestion Kafka stream. The schema is similar
//! to the aggregation payload, with the addition of scoping information. Each bucket is sent in a
//! separate message:
//!
//! ```json
#![doc = include_str!("../tests/fixtures/kafka.json")]
//! ```
.

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 will definitely do this in a follow up, I also want to get a proper documentation in for how the limiter works!

@Dav1dde Dav1dde enabled auto-merge (squash) December 12, 2023 08:11
@Dav1dde Dav1dde disabled auto-merge December 12, 2023 08:29
relay-cardinality/Cargo.toml Outdated Show resolved Hide resolved
relay-cardinality/src/limiter.rs Outdated Show resolved Hide resolved
relay-config/src/config.rs Show resolved Hide resolved
@Dav1dde Dav1dde enabled auto-merge (squash) December 12, 2023 09:00
@Dav1dde Dav1dde merged commit 82b8980 into master Dec 12, 2023
20 checks passed
@Dav1dde Dav1dde deleted the feat/cardinality-limiter branch December 12, 2023 09:13
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.

3 participants