feat(cardinality): Create outcomes for cardinality limited metrics #2947
Conversation
Dav1dde
force-pushed
the
feat/cardlimit-outcomes
branch
from
7637fc9
to
ace8aeb
Compare
| @@ -180,6 +183,7 @@ impl Outcome { | |||
| match self { | |||
| Outcome::Filtered(_) | Outcome::FilteredSampling(_) => OutcomeId::FILTERED, | |||
| Outcome::RateLimited(_) => OutcomeId::RATE_LIMITED, | |||
| Outcome::CardinalityLimited => OutcomeId::RATE_LIMITED, | |||
There was a problem hiding this comment.
Should we introduce a new outcome instead?
There was a problem hiding this comment.
It's not strictly necessary; we could also differentiate it purely by reason code, which is what you're doing now. While it's not ideal that two variants to the same outcome ID, we already have a similar setup for dynamic sampling.
If you were to introduce a new outcome, please split this PR in two parts, and first bump all libraries to avoid errors in the consumers.
There was a problem hiding this comment.
I think it'd be interesting to have this data available internally to evaluate how high/low the limits are, and the impact on costs. I don't know if other parts (e.g. consumers?) need to be updated to handle new outcomes.
There was a problem hiding this comment.
👍 let's keep it like this and in the future we can split this into a separate outcome id
Dav1dde
force-pushed
the
feat/cardlimit-outcomes
branch
from
ace8aeb
to
3c82ebc
Compare
| @@ -180,6 +183,7 @@ impl Outcome { | |||
| match self { | |||
| Outcome::Filtered(_) | Outcome::FilteredSampling(_) => OutcomeId::FILTERED, | |||
| Outcome::RateLimited(_) => OutcomeId::RATE_LIMITED, | |||
| Outcome::CardinalityLimited => OutcomeId::RATE_LIMITED, | |||
There was a problem hiding this comment.
It's not strictly necessary; we could also differentiate it purely by reason code, which is what you're doing now. While it's not ideal that two variants to the same outcome ID, we already have a similar setup for dynamic sampling.
If you were to introduce a new outcome, please split this PR in two parts, and first bump all libraries to avoid errors in the consumers.
| let mode = match project_state.config.transaction_metrics { | ||
| Some(ErrorBoundary::Ok(ref c)) if c.usage_metric() => ExtractionMode::Usage, | ||
| _ => ExtractionMode::Duration, | ||
| }; | ||
|
|
||
| if project_state.has_feature(Feature::CardinalityLimiter) { | ||
| buckets = self.cardinality_limit_buckets(scoping, buckets, mode); |
| @@ -180,6 +183,7 @@ impl Outcome { | |||
| match self { | |||
| Outcome::Filtered(_) | Outcome::FilteredSampling(_) => OutcomeId::FILTERED, | |||
| Outcome::RateLimited(_) => OutcomeId::RATE_LIMITED, | |||
| Outcome::CardinalityLimited => OutcomeId::RATE_LIMITED, | |||
There was a problem hiding this comment.
I think it'd be interesting to have this data available internally to evaluate how high/low the limits are, and the impact on costs. I don't know if other parts (e.g. consumers?) need to be updated to handle new outcomes.
Dav1dde
changed the title
Creates rate limited outcomes for cardinality limited items.
Maybe we should introduce a new outcome id for the cardinality limiter instead?