feat(metrics): Limits on bucketing cost in aggregator [INGEST-1132] #1287
Conversation
Add new settings to enforce cost limits per project and process.
| // XXX: This is not a great implementation of cost enforcement. | ||
| // | ||
| // * it takes two lookups of the project key in the cost tracker to merge a bucket: once in | ||
| // `check_limits_exceeded` and once in `add_cost`. |
There was a problem hiding this comment.
Could you try to return the error from add_cost and do the diff computation inline? The .entry() function below doesn't create the entry until you call insert on it at least, which just leaves the merge into the bucket, and that could also easily return whether it did insert or not.
It's excellent that you're calling out caveats of enforcement below, though in practice an off-by-one is not an issue.
| // Another consequence is that a MergeValue that adds zero cost (such as an existing | ||
| // counter bucket being incremented) is currently rejected even though it doesn't have to | ||
| // be. |
There was a problem hiding this comment.
That is actually problematic and we should fix that. Same as in my previous comment -- if merge_into returns what happened, you should be able to enforce more consistently. See also #1284 (comment).
There was a problem hiding this comment.
If I call merge_into, it's already too late to enforce anything, as I can't potentially undo a merge. I would need to special-case counters or add a method to MergeValue that returns projected added cost under either occupied/vacant scenarios.
let's check in on monday
There was a problem hiding this comment.
That's OK, since worst case you're off by one, so depending on the value you're inserting you're less than 48 bytes over. For a memory limit of 16GB that is insignificant. The other option is to implement cost on MetricValue.
There was a problem hiding this comment.
Let's talk on monday, everything you're saying is true but I don't think it implies that one can fix this case (without impl-ing cost on MetricValue)
There was a problem hiding this comment.
Not sure if implementing cost on MetricValue solves the problem. Whether for example a set bucket grows depends on the pre-existing set entries, not just the incoming value. So to be 100% consistent we would need a function that answers the question "would this increase my size?", which would be overkill IMO.
| /// Maximum amount of bytes used for metrics aggregation per project. | ||
| /// | ||
| /// Similar measuring technique to `max_total_bucket_bytes`, but instead of a | ||
| /// global/process-wide limit, it is enforced per project id. |
There was a problem hiding this comment.
| /// global/process-wide limit, it is enforced per project id. | |
| /// global/process-wide limit, it is enforced per project key. |
There was a problem hiding this comment.
Can we still fix this and also use a better config name?
There was a problem hiding this comment.
What name would you suggest? max_bucket_bytes_total and max_bucket_bytes_per_project_key?
| // Another consequence is that a MergeValue that adds zero cost (such as an existing | ||
| // counter bucket being incremented) is currently rejected even though it doesn't have to | ||
| // be. |
There was a problem hiding this comment.
Not sure if implementing cost on MetricValue solves the problem. Whether for example a set bucket grows depends on the pre-existing set entries, not just the incoming value. So to be 100% consistent we would need a function that answers the question "would this increase my size?", which would be overkill IMO.
|
I think all comments should've been addressed now. We talked it through async and determined that consistent enforcement and accepting counter increments (because they don't have cost) is not worth the effort. |
* 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)
Add new settings to enforce cost limits per project and process.