Distributor: stop counting metadata towards the ingestion rate limit

[friedrichg]

What this PR does:
-distributor.ingestion-rate-limit is documented as a limit in samples per second, but metadata was counted in the same budget as samples and exemplars. Metadata is not sample data, and its volume is already bounded by -ingester.max-metadata-per-user and -ingester.max-metadata-per-metric.

This applies to both remote write 1.0 and 2.0, since the limit is enforced on the shared Distributor.Push path. 2.0 is affected most, as it attaches metadata to every series, so a tenant consumed roughly twice its configured limit for the same data. But 1.0 senders are affected too: Prometheus sends metadata by default (metadata_config.send, up to 2000 entries per request every minute), and those metadata-only requests are now unrated. The ingester instance rate stops counting metadata too, for consistency (its comment already said it mirrored the distributor).

Opened as a draft alternative to #7760, which dedupes metadata during PRW2 conversion instead. Note this PR removes #7760's stated motivation (the ~2x rate limit consumption); #7760's remaining benefits are fewer allocations and less distributor-to-ingester volume, which are real but are efficiency wins rather than a limit-correctness fix. Happy to close this if the dedup is preferred.

Upstream Prometheus takes the same position on the sender side: metadata is not a sample and is not counted against max_samples_per_send (prometheus/prometheus#14407).

Which issue(s) this PR fixes:
N/A

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated
• docs/configuration/v1-guarantees.md updated if this PR introduces experimental flags

[friedrichg]

Scope note: this affects remote write 1.0 too, and the ~2x argument does not transfer

The justification differs per protocol, so it is worth being precise.

Remote write 2.0 is the amplification case. Every TimeSeries carries a Metadata block, so a family with N series contributes N copies of the same metadata to totalN. That is where the roughly 2x consumption comes from.

Remote write 1.0 has no amplification. Prometheus sends metadata once per family, batched, on a timer (DefaultMetadataConfig: Send: true, MaxSamplesPerSend: 2000, SendInterval: 1m), over a path separate from samples (QueueManager.AppendWatcherMetadata -> sendMetadataWithBackoff). So the v1 case rests on different points:

1. Contract. -distributor.ingestion-rate-limit is documented "in samples per second" and -distributor.ingestion-burst-size "in number of samples". Metadata is not samples.
2. Burst interaction. Metadata is bursty rather than smooth. A tenant with tens of thousands of metric families flushes that many tokens in a short window every minute, against a default burst of 50000. That can produce 429s on genuine sample writes which happen to coincide with the flush.
3. The wrong limiter rejects it. v1 metadata arrives as metadata-only requests, which consumed sample budget while carrying no samples, and could be 429'd by a sample-oriented limit, causing sender retries. Metadata already has dedicated bounds (-ingester.max-metadata-per-user, -ingester.max-metadata-per-metric), so it was double-bounded and the inappropriate bound was doing the rejecting.

Trade-off I would like input on

This removes rate bounding from the metadata path entirely, for both protocols. Since metadata_config.send defaults to true, a stock v1 sender now pushes up to 2000 metadata entries per request every minute with no rate bound at all, leaving only the cardinality limits above.

So for v1 this is closer to a principled cleanup plus a burst fix than to a bug fix, and a reasonable alternative is a dedicated metadata rate limiter rather than excluding metadata from the sample budget. I kept this PR to the simple exclusion because it matches the flag's documented contract and the upstream position that metadata is not a sample (prometheus/prometheus#14407), but I am happy to add a separate limiter instead if reviewers would rather keep backpressure on that path.

[friedrichg]

Follow-up: this also removes metadata from the instance-level overload valves

Looking at the blast radius more carefully, this change reaches wider than the per-tenant limit, and I do not think all of it is intended.

Both instance-level valves read the same EWMA counters this PR stops feeding:

• pkg/distributor/distributor.go:801: if rate := d.ingestionRate.Rate(); rate >= d.cfg.InstanceLimits.MaxIngestionRate returns 503
• pkg/ingester/ingester.go:1377: if rate := i.ingestionRate.Rate(); rate >= il.MaxIngestionRate returns errMaxSamplesPushRateLimitReached

Since d.ingestionRate.Add(totalN) and its ingester equivalent no longer include metadata, metadata becomes invisible to -distributor.instance-limits.max-ingestion-rate and -ingester.instance-limits.max-ingestion-rate. Both default to 0 (unlimited), so default deployments are unaffected, but operators who do set them lose metadata from that accounting.

That is a different decision from the per-tenant one. The per-tenant limit is documented in samples per second, so excluding metadata matches its stated contract. The instance limits exist to keep a process from being overrun, where the relevant question is work performed rather than sample semantics.

What a metadata flood costs after this PR

Cardinality and memory are still bounded at the ingester: AssertMaxMetricsWithMetadataPerUser (default 8000 families) and AssertMaxMetadataPerMetric (default 10 per metric) reject beyond the limit and increment DiscardedMetadata.

The realistic flood is duplicates, since senders re-send the same metadata every interval. That path passes both asserts and ends at mm.metricToMetadata[metric][*metadata] = time.Now() in pkg/ingester/user_metrics_metadata.go, so there is no memory growth and nothing is discarded. But add() takes a per-tenant exclusive mm.mtx.Lock() once per metadata entry, contending with purge() and with the MetricsMetadata read API. After this PR nothing rate-shaped bounds that work. What remains is only concurrency and size shaped: max-inflight-push-requests, max-inflight-client-requests, maxRecvMsgSize, and the per-entry ValidateMetadata length checks.

Options

1. Narrow this PR: keep metadata out of the per-tenant sample rate limit, but keep feeding d.ingestionRate and i.ingestionRate so the instance valves still account for it. Smallest change, and probably the correct split.
2. Add a dedicated metadata rate limiter and keep metadata fully out of the sample budget.

I lean towards option 1 here, with option 2 as a follow-up if operators want explicit metadata backpressure. Happy to push either.

[friedrichg]

Correction: the documentation should match the behaviour, not the reverse

I am withdrawing the direction of this PR and of my previous two comments.

-distributor.ingestion-rate-limit is described as "Per-user ingestion rate limit in samples per second" and -distributor.ingestion-burst-size as "Per-user allowed ingestion burst size (in number of samples)". The implementation has always counted samples, exemplars and metadata together in totalN. The mismatch is in the wording, not in the accounting: this limit is a total ingestion budget, and describing it as samples-only is the inaccurate part.

So the fix should be documentation only. Changing the accounting to satisfy the wording is the wrong way round, and it carries real downsides that I raised on this PR myself:

• it silently raises effective limits for every existing deployment, on both remote write 1.0 and 2.0
• it leaves the metadata path with no rate bound at all
• it removes metadata from the instance-level overload valves (-distributor.instance-limits.max-ingestion-rate and the ingester equivalent), which was never intended

None of that is worth it to make a help string true when the help string is the thing that is wrong.

Consequence for #7760: with the accounting unchanged, the remote write 2.0 amplification that PR describes stays a real problem, and deduplicating per-series metadata during conversion is an appropriate fix for it. I withdraw my earlier claim that this PR removes #7760's motivation. Apologies for the noise there.

This PR should therefore be dropped in favour of a documentation-only change to the two flag descriptions, with the generated config reference regenerated to match.

[friedrichg]

Closing per the correction above: the accounting is intentional and this limit is a total ingestion budget, so the fix belongs in the flag descriptions rather than in the behaviour. Will follow up with a documentation-only change to -distributor.ingestion-rate-limit and -distributor.ingestion-burst-size.

The remote write 2.0 metadata amplification remains a real problem, and #7760 is an appropriate fix for it.