fix(metrics): Track memory footprint more accurately [INGEST-1132] #1288
Conversation
| // Choosing a BTreeMap instead of a HashMap here, under the assumption that a BTreeMap | ||
| // is still more efficient for the number of project keys we store. | ||
| cost_per_project_key: BTreeMap<ProjectKey, usize>, | ||
| cost_per_project_key: HashMap<ProjectKey, usize>, |
There was a problem hiding this comment.
I did not run any benchmarks, but figured that a HashMap is a safer choice w.r.t. scaling.
There was a problem hiding this comment.
it's fine either way. btreemap might have an edge for small values
| @@ -1287,7 +1339,7 @@ impl Aggregator { | |||
|
|
|||
| let flush_at = self.config.get_flush_time(timestamp, project_key); | |||
| let bucket = value.into(); | |||
| added_cost = bucket.cost(); | |||
| added_cost = entry.key().cost() + bucket.cost(); | |||
There was a problem hiding this comment.
While at it, it would be good to deal with the Occupied branch too. See #1287 (comment)
There was a problem hiding this comment.
The Occupied branch is handled correctly (though not elegantly):
relay/relay-metrics/src/aggregation.rs
Lines 1303 to 1306 in 5764c12
The advantage of computing the cost twice and subtracting is that we only need a single function for computing bucket value cost. If we did something like added_cost = if something_added { value.incremental_cost() } else { 0 }, we would need to also implement an incremental_cost function on BucketValue and MetricValue.
| // Choosing a BTreeMap instead of a HashMap here, under the assumption that a BTreeMap | ||
| // is still more efficient for the number of project keys we store. | ||
| cost_per_project_key: BTreeMap<ProjectKey, usize>, | ||
| cost_per_project_key: HashMap<ProjectKey, usize>, |
There was a problem hiding this comment.
it's fine either way. btreemap might have an edge for small values
relay-metrics/src/aggregation.rs
Outdated
| let value = std::mem::replace(&mut entry.value, BucketValue::Counter(0.0)); | ||
| cost_tracker.subtract_cost(key.project_key, value.cost()); | ||
| let value = mem::replace(&mut entry.value, BucketValue::Counter(0.0)); | ||
| cost_tracker.subtract_cost(key.project_key, key.cost() + value.cost()); |
There was a problem hiding this comment.
can we call subtract_cost twice instead? then overflow/underflow handling is also dealt with in one place
* master: ref(metrics): Stop logging relative bucket size (#1302) fix(metrics): Rename misnamed aggregator option (#1298) fix(server): Avoid a panic in the Sentry middleware (#1301) build: Update dependencies with known vulnerabilities (#1294) fix(metrics): Stop logging statsd metric per project key (#1295) feat(metrics): Limits on bucketing cost in aggregator [INGEST-1132] (#1287) fix(metrics): Track memory footprint more accurately (#1288) build(deps): Bump dependencies (#1293) feat(aws): Add relay-aws-extension crate which implements AWS extension as an actor (#1277) fix(meta): Update codeowners for the release actions (#1286) feat(metrics): Track memory footprint of metrics buckets (#1284)
#1284 introduced a cost model for measuring the memory footprint of metrics buckets stored in the aggregator. It has two flaws:
BucketValue(only looked at the values inside).BucketKey.This PR attempts to fix both issues.