google / google/mtail

Enforce metric `limit` at datum creation? (GetDatum has a // TODO Check m.Limit)

Open
#1,006 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.