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!
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 checkm.Limitfirst, it just has a// TODO Check m.Limit and expire old data. So the cap is only applied later byStore.Gc, and only whenLimit > 0. With the defaultLimit == 0there'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
LimitatGetDatumtime (evict-oldest, or skip-create when at the cap)? It's already insidem.Lockso 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!