Skip to content

Introduce max_series_per_metric limitation - #265

Merged
Watson1978 merged 3 commits into
fluent:masterfrom
kenhys:fix-max-series-per-metric
Aug 28, 2026
Merged

Introduce max_series_per_metric limitation#265
Watson1978 merged 3 commits into
fluent:masterfrom
kenhys:fix-max-series-per-metric

Conversation

@kenhys

@kenhys kenhys commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

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.

@kenhys

kenhys commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

It should be rebased after #264 was merged.

@kenhys
kenhys force-pushed the fix-max-series-per-metric branch 3 times, most recently from 3478a5b to eeb8ae2 Compare August 27, 2026 09:17
@kenhys
kenhys marked this pull request as ready for review August 27, 2026 09:17
@kenhys
kenhys requested a review from Watson1978 August 27, 2026 09:25
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>
@kenhys

kenhys commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

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)

@Watson1978 Watson1978 Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
end

https://github.com/prometheus/client_ruby/blob/9c452538f4c9e06fb32a4fa5acc903652227bc52/lib/prometheus/client/summary.rb#L20-L27

Suggestion:

if value
  value = Float(value)
  with_label_set(record, expander) do |label|
    @summary.observe(value, labels: label)
  end
end

Float() 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As value=Float(value) passthough numeric string, it is behavior change. so changed to validate it before entering with_label_set.

Comment thread README.md
Comment on lines +282 to +286
**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`:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

kenhys and others added 2 commits August 28, 2026 06:24
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>
@kenhys
kenhys force-pushed the fix-max-series-per-metric branch from eeb8ae2 to f17543e Compare August 28, 2026 06:27

@Watson1978 Watson1978 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@Watson1978
Watson1978 merged commit 48beb77 into fluent:master Aug 28, 2026
9 checks passed
@kenhys
kenhys deleted the fix-max-series-per-metric branch August 28, 2026 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants