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: Replace organization id type with newtype #4159

Merged
merged 13 commits into from
Nov 12, 2024
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
- Feature flags of graduated features are now hard-coded in Relay so they can be removed from Sentry. ([#4076](https://github.com/getsentry/relay/pull/4076), [#4080](https://github.com/getsentry/relay/pull/4080))
- Add parallelization in Redis commands. ([#4118](https://github.com/getsentry/relay/pull/4118))
- Extract user ip for spans. ([#4144](https://github.com/getsentry/relay/pull/4144))
- Replace u64 with `OrganizationId` new-type struct for organization id. ([#4159](https://github.com/getsentry/relay/pull/4159))
Litarnus marked this conversation as resolved.
Show resolved Hide resolved

## 24.9.0

Expand Down
1 change: 1 addition & 0 deletions relay-base-schema/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@
pub mod data_category;
pub mod events;
pub mod metrics;
pub mod organization;
pub mod project;
pub mod spans;
27 changes: 27 additions & 0 deletions relay-base-schema/src/organization.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//! Contains [`OrganizationId`] which is the ID of a Sentry organization and is currently a
//! a wrapper over `u64`.

use serde::{Deserialize, Serialize};

/// The unique identifier of a Sentry organization
Litarnus marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Copy, Clone, Debug, PartialEq, Eq, Ord, PartialOrd, Hash, Serialize, Deserialize)]
pub struct OrganizationId(u64);
Litarnus marked this conversation as resolved.
Show resolved Hide resolved

impl OrganizationId {
/// Creates a new organization ID from its numeric value
#[inline]
pub fn new(id: u64) -> Self {
OrganizationId(id)
Litarnus marked this conversation as resolved.
Show resolved Hide resolved
}

/// returns the numeric value of the organization ID
Litarnus marked this conversation as resolved.
Show resolved Hide resolved
pub fn value(self) -> u64 {
self.0
}
}

impl std::fmt::Display for OrganizationId {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.value())
}
}
3 changes: 2 additions & 1 deletion relay-cardinality/benches/redis_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::{
use criterion::{BatchSize, BenchmarkId, Criterion};
use relay_base_schema::{
metrics::{MetricName, MetricNamespace},
organization::OrganizationId,
project::ProjectId,
};
use relay_cardinality::{
Expand Down Expand Up @@ -83,7 +84,7 @@ impl Params {
namespace: None,
}],
scoping: Scoping {
organization_id: 1,
organization_id: OrganizationId::new(1),
project_id: ProjectId::new(100),
},
rounds,
Expand Down
3 changes: 0 additions & 3 deletions relay-cardinality/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,3 @@ pub use self::window::SlidingWindow;
/// Redis Set based cardinality limiter.
#[cfg(feature = "redis")]
pub type CardinalityLimiter = self::limiter::CardinalityLimiter<RedisSetLimiter>;

/// Internal alias for better readability.
type OrganizationId = u64;
5 changes: 3 additions & 2 deletions relay-cardinality/src/limiter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ use std::collections::BTreeMap;

use hashbrown::{HashMap, HashSet};
use relay_base_schema::metrics::{MetricName, MetricNamespace, MetricType};
use relay_base_schema::organization::OrganizationId;
use relay_base_schema::project::ProjectId;
use relay_common::time::UnixTimestamp;
use relay_statsd::metric;

use crate::statsd::CardinalityLimiterTimers;
use crate::{CardinalityLimit, Error, OrganizationId, Result};
use crate::{CardinalityLimit, Error, Result};

/// Data scoping information.
///
Expand Down Expand Up @@ -394,7 +395,7 @@ mod tests {

fn build_scoping() -> Scoping {
Scoping {
organization_id: 1,
organization_id: OrganizationId::new(1),
project_id: ProjectId::new(1),
}
}
Expand Down
13 changes: 7 additions & 6 deletions relay-cardinality/src/redis/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,11 +197,12 @@ impl ScopedCache {
#[cfg(test)]
mod tests {
use relay_base_schema::metrics::{MetricName, MetricNamespace};
use relay_base_schema::organization::OrganizationId;
use relay_base_schema::project::ProjectId;

use crate::limiter::{Entry, EntryId};
use crate::redis::quota::PartialQuotaScoping;
use crate::{CardinalityLimit, CardinalityScope, OrganizationId, Scoping, SlidingWindow};
use crate::{CardinalityLimit, CardinalityScope, Scoping, SlidingWindow};

use super::*;

Expand Down Expand Up @@ -238,7 +239,7 @@ mod tests {
window_seconds: 100,
granularity_seconds: 10,
};
let scope = build_scoping(1, window);
let scope = build_scoping(OrganizationId::new(1), window);
let now = UnixTimestamp::now();
let future = now + Duration::from_secs(window.granularity_seconds + 1);

Expand Down Expand Up @@ -298,8 +299,8 @@ mod tests {
window_seconds: 100,
granularity_seconds: 10,
};
let scope1 = build_scoping(1, window);
let scope2 = build_scoping(2, window);
let scope1 = build_scoping(OrganizationId::new(1), window);
let scope2 = build_scoping(OrganizationId::new(2), window);

let now = UnixTimestamp::now();

Expand Down Expand Up @@ -336,8 +337,8 @@ mod tests {
window_seconds: vacuum_interval.as_secs() * 10,
granularity_seconds: vacuum_interval.as_secs() * 2,
};
let scope1 = build_scoping(1, window);
let scope2 = build_scoping(2, window);
let scope1 = build_scoping(OrganizationId::new(1), window);
let scope2 = build_scoping(OrganizationId::new(2), window);

let now = UnixTimestamp::now();
let in_interval = now + Duration::from_secs(vacuum_interval.as_secs() - 1);
Expand Down
9 changes: 6 additions & 3 deletions relay-cardinality/src/redis/limiter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ mod tests {
use std::sync::atomic::AtomicU64;

use relay_base_schema::metrics::{MetricName, MetricNamespace::*, MetricType};
use relay_base_schema::organization::OrganizationId;
use relay_base_schema::project::ProjectId;
use relay_redis::{redis, RedisConfigOptions};

Expand Down Expand Up @@ -293,7 +294,9 @@ mod tests {
static ORGS: AtomicU64 = AtomicU64::new(100);

let scoping = Scoping {
organization_id: ORGS.fetch_add(1, std::sync::atomic::Ordering::SeqCst),
organization_id: OrganizationId::new(
ORGS.fetch_add(1, std::sync::atomic::Ordering::SeqCst),
),
project_id: ProjectId::new(1),
};

Expand Down Expand Up @@ -593,12 +596,12 @@ mod tests {
let granularity_seconds = 10_000;

let scoping1 = Scoping {
organization_id: granularity_seconds,
organization_id: OrganizationId::new(granularity_seconds),
project_id: ProjectId::new(1),
};
let scoping2 = Scoping {
// Time shift relative to `scoping1` should be half the granularity.
organization_id: granularity_seconds / 2,
organization_id: OrganizationId::new(granularity_seconds / 2),
project_id: ProjectId::new(1),
};

Expand Down
7 changes: 4 additions & 3 deletions relay-cardinality/src/redis/quota.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@ use std::fmt::{self, Write};
use std::hash::Hash;

use relay_base_schema::metrics::{MetricName, MetricNamespace, MetricType};
use relay_base_schema::organization::OrganizationId;
use relay_base_schema::project::ProjectId;
use relay_common::time::UnixTimestamp;

use crate::limiter::Entry;
use crate::redis::{KEY_PREFIX, KEY_VERSION};
use crate::window::Slot;
use crate::{CardinalityLimit, CardinalityScope, OrganizationId, Scoping, SlidingWindow};
use crate::{CardinalityLimit, CardinalityScope, Scoping, SlidingWindow};

/// A quota scoping extracted from a [`CardinalityLimit`] and a [`Scoping`].
///
Expand Down Expand Up @@ -70,7 +71,7 @@ impl PartialQuotaScoping {
fn shifted(&self, timestamp: UnixTimestamp) -> UnixTimestamp {
let shift = self
.organization_id
.map(|o| o % self.window.granularity_seconds)
.map(|o| o.value() % self.window.granularity_seconds)
.unwrap_or(0);

UnixTimestamp::from_secs(timestamp.as_secs() + shift)
Expand Down Expand Up @@ -119,7 +120,7 @@ impl QuotaScoping {

/// Turns the scoping into a Redis key for the passed slot.
pub fn to_redis_key(&self, slot: Slot) -> String {
let organization_id = self.organization_id.unwrap_or(0);
let organization_id = self.organization_id.unwrap_or(OrganizationId::new(0));
let project_id = self.project_id.map(|p| p.value()).unwrap_or(0);
let namespace = self.namespace.map(|ns| ns.as_str()).unwrap_or("");
let metric_type = DisplayOptMinus(self.metric_type);
Expand Down
4 changes: 2 additions & 2 deletions relay-cardinality/src/redis/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,10 @@ impl<'a> Drop for LimitState<'a> {
passive = passive,
);

let organization_id = self.scoping.organization_id as i64;
let organization_id = self.scoping.organization_id;
let status = if self.rejections > 0 { "limited" } else { "ok" };
metric!(
set(CardinalityLimiterSets::Organizations) = organization_id,
set(CardinalityLimiterSets::Organizations) = organization_id.value() as i64,
id = &self.cardinality_limit.id,
passive = passive,
status = status,
Expand Down
7 changes: 4 additions & 3 deletions relay-metrics/src/meta/redis.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::time::Duration;

use hash32::{FnvHasher, Hasher as _};
use relay_base_schema::organization::OrganizationId;
use relay_base_schema::project::ProjectId;
use relay_common::time::UnixTimestamp;
use relay_redis::{RedisError, RedisPool};
Expand All @@ -23,7 +24,7 @@ impl RedisMetricMetaStore {
/// Stores metric metadata in Redis.
pub fn store(
&self,
organization_id: u64,
organization_id: OrganizationId,
project_id: ProjectId,
meta: MetricMeta,
) -> Result<(), RedisError> {
Expand Down Expand Up @@ -67,7 +68,7 @@ impl RedisMetricMetaStore {

fn build_redis_key(
&self,
organization_id: u64,
organization_id: OrganizationId,
project_id: ProjectId,
timestamp: UnixTimestamp,
mri: &MetricResourceIdentifier<'_>,
Expand Down Expand Up @@ -113,7 +114,7 @@ mod tests {
fn test_store() {
let store = build_store();

let organization_id = 1000;
let organization_id = OrganizationId::new(1000);
let project_id = ProjectId::new(2);
let mri = MetricResourceIdentifier::parse("c:foo").unwrap();

Expand Down
3 changes: 2 additions & 1 deletion relay-quotas/src/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ mod tests {
use super::*;

use relay_base_schema::data_category::DataCategory;
use relay_base_schema::organization::OrganizationId;
use relay_base_schema::project::{ProjectId, ProjectKey};
use relay_common::time::UnixTimestamp;
use relay_redis::{RedisConfigOptions, RedisPool};
Expand Down Expand Up @@ -273,7 +274,7 @@ mod tests {

fn build_scoping() -> Scoping {
Scoping {
organization_id: 69420,
organization_id: OrganizationId::new(69420),
project_id: ProjectId::new(42),
project_key: ProjectKey::parse("a94ae32be2584e0bbd7a4cbb95971fee").unwrap(),
key_id: Some(4711),
Expand Down
Loading
Loading