Introduce max_series_per_metric limitation - #265
Conversation
|
It should be rebased after #264 was merged. |
3478a5b to
eeb8ae2
Compare
Before: a metric held a series for every distinct label set a record expanded to. In one experiment, about 8 million records took the RSS from 64MB to 582MB, which is an OOM DoS a client can drive. After: max_series_per_metric drops a record which brings a new label set once the metric holds that many, and the same experiment ends at 84MB instead. Set it on the plugin, or per <metric>. It defaults to 0 (no limit), because a drop loses the record for good. The label sets are counted per client metric, so <metric> sections with the same name share one count. A slot is taken before instrumenting and given back on failure, so that concurrent records cannot both pass the limit and a record which fails to be instrumented does not consume it. A drop gets a throttled warning and is counted in fluentd_prometheus_dropped_label_sets_total. Signed-off-by: Kentaro Hayashi <hayashi@clear-code.com> Co-Authored-By: Claude <noreply@anthropic.com>
|
Consistency with initlabels should be fixed in follow-up #266 |
| if value | ||
| @summary.observe(value, labels: labels(record, expander)) | ||
| with_label_set(record, expander) do |label| | ||
| @summary.observe(value, labels: label) |
There was a problem hiding this comment.
with_label_set gives the reserved slot back whenever the block raises, on the
assumption that the client call is all-or-nothing. Summary#observe breaks that
assumption:
def observe(value, labels: {})
base_label_set = label_set_for(labels)
@store.synchronize do
@store.increment(labels: base_label_set.merge(quantile: "count"), by: 1)
@store.increment(labels: base_label_set.merge(quantile: "sum"), by: value)
end
endSuggestion:
if value
value = Float(value)
with_label_set(record, expander) do |label|
@summary.observe(value, labels: label)
end
endFloat() raises before with_label_set, so a bad record never takes a slot and
still reaches @ERROR. I would not use to_f here: it turns "-", "" and
"abc" into 0.0, which silently pollutes the observations instead of
surfacing them.
There was a problem hiding this comment.
As value=Float(value) passthough numeric string, it is behavior change. so changed to validate it before entering with_label_set.
| **The limit is disabled by default and must be enabled explicitly**, since a | ||
| dropped record is lost and cannot be recovered. A `<metric>` section overrides | ||
| the value given to the plugin, so that a metric which expands faster than the | ||
| others is bound on its own, while one whose labels are known to be bounded | ||
| stays unlimited with `0`: |
There was a problem hiding this comment.
Could this section also note that the limit is per worker process? @registry is
::Prometheus::Client.registry, which is process local, so with workers N a
metric can hold up to N times max_series_per_metric label sets in total.
Before: the client raised on such a value once with_label_set had reserved the slot. Summary raised only after Summary#observe had incremented its count, so releasing the slot left a half instrumented series which the metric could not take back. After: the value is validated before with_label_set, so a record which cannot be instrumented never takes a slot and never reaches the client. It is still routed to @error as before. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Kentaro Hayashi <hayashi@clear-code.com>
Signed-off-by: Kentaro Hayashi <hayashi@clear-code.com>
eeb8ae2 to
f17543e
Compare
Before:
a metric held one label set per distinct label value a record
carried. About 8 million records took the RSS from 64MB to 582MB, which
is an OOM DoS a client can drive.
After:
max_series_per_metric drops a new label set once the metric holds
that many of them, and the same records take the RSS from 64MB to 84MB.
Set it on the plugin, or per . It defaults to 0 (no limit),
because a drop loses the record for good.