feat(spans): Rate limit span metrics #3255
Conversation
| *profiles += other.profiles; | ||
| *buckets = other.buckets; | ||
| *buckets += other.buckets; |
There was a problem hiding this comment.
I messed this up in the PR that was supposed to fix it.
| count, | ||
| over_accept_once, | ||
| ) | ||
| .ok() // don't limit if there was a redis error |
There was a problem hiding this comment.
This is an important change: Previously, we would drop everything if there was a redis error. I changed this to accept everything instead, for two reasons:
- It is more correct IMO (we should not drop customer data if the current budget is unknown).
- It simplifies the code.
There was a problem hiding this comment.
nit: adding point 1 as a comment in the code is valuable (same for spans below).
jjbayer
changed the title
| count, | ||
| over_accept_once, | ||
| ) | ||
| .ok() // don't limit if there was a redis error |
There was a problem hiding this comment.
nit: adding point 1 as a comment in the code is valuable (same for spans below).
| /// The number of transactions contributing to these metrics. | ||
| transaction_count: usize, | ||
|
|
||
| /// The number of profiles contained in these metrics. | ||
| profile_count: usize, | ||
| counts: TotalEntityCounts, |
There was a problem hiding this comment.
counts also counts spans now, so something like:
-/// The number of transactions contributing to these metrics.
+/// The number of performance items (transactions, spans) contributing to these metrics.(can't make a suggestion on a life outside of the diff)
| } | ||
| }; | ||
|
|
||
| if mri.namespace != MetricNamespace::Transactions { | ||
| return None; | ||
| if mri.namespace == MetricNamespace::Transactions { |
There was a problem hiding this comment.
Could we make match mri.namespace instead?
| @@ -45,30 +37,34 @@ const PROFILE_TAG: &str = "has_profile"; | |||
| /// Additionally tracks whether the transactions also contained profiling information. | |||
| /// | |||
| /// Returns `None` if the metric was not extracted from transactions. | |||
| fn count_metric_bucket(metric: BucketView<'_>, mode: ExtractionMode) -> Option<TransactionCount> { | |||
| fn count_metric_bucket(metric: BucketView<'_>, mode: ExtractionMode) -> BucketSummary { | |||
There was a problem hiding this comment.
Probably can get a better name now, something something summary?
There was a problem hiding this comment.
How about BucketSummary::new?
There was a problem hiding this comment.
Renamed to summarize_bucket to keep the diff small.
| .iter() | ||
| .map(|metric| count_metric_bucket(BucketView::new(metric), mode)) | ||
| let buckets: Vec<_> = buckets | ||
| .into_iter() |
There was a problem hiding this comment.
Simplifies code, but this results in an additional allocation, which is probably fine, just wanna bring it up.
There was a problem hiding this comment.
Does it? into_iter() takes ownership of the existing vector AFAIK.
There was a problem hiding this comment.
into_iter() takes ownership, but each element of the vector changes size -> needs an allocation, later we extract the buckets again -> another allocation. Instead of just having 1 allocation for the extra data.
| /// | ||
| /// We do not explicitly check the rate limiter for profiles, so there is no need to | ||
| /// distinguish between `None` and `Some(0)`. | ||
| profiles: usize, |
There was a problem hiding this comment.
baby nit: Maybe have the profiles part of the transaction count transactions: Option<TransactionCountWithProfiles>?
Like we do for transaction metrics, limit span metrics when there is a quota defined for
DataCategory::Spans.Fixes: #3260