Enforce metric `limit` at datum creation? (GetDatum has a // TODO Check m.Limit)
- Dominant language
- Go
- Stars
- 4k
- Forks
- 392
- PR merge metrics
- No merged PRs in 30d
Description
Hi @jaqx0r! Wanted to check the intended design before sending a patch.
`Metric.GetDatum` (`internal/metrics/metric.go`) creates a new datum on a miss but doesn't check `m.Limit` first, it just has a `// TODO Check m.Limit and expire old data`. So the cap is only applied later by `Store.Gc`, and only when `Limit > 0`. With the default `Limit == 0` there's no cap at all, so a program that labels a metric by a high-cardinality field (request path, user-agent, etc.) can grow label cardinality without bound between GC cycles. That lines up with the high-cardinality behaviour you were poking at in #1003 and the older #61.
Would you take a patch that enforces `Limit` at `GetDatum` time (evict-oldest, or skip-create when at the cap)? It's already inside `m.Lock` so it's cheap, but it is the hot path, so I wanted to ask before writing it. Happy to include a benchmark showing the added cost, and to leave the default (0 = unbounded) as-is unless you'd want a sane default.
Thanks!
Contributor guide
Assessment
This issue has not been assessed yet.