Skip to content

Ingester: account for duplicate samples silently dropped at TSDB commit - #16347

Draft
dmatth1 wants to merge 2 commits into
mainfrom
dan/ingester-duplicate-sample-stats
Draft

Ingester: account for duplicate samples silently dropped at TSDB commit#16347
dmatth1 wants to merge 2 commits into
mainfrom
dan/ingester-duplicate-sample-stats

Conversation

@dmatth1

@dmatth1 dmatth1 commented Aug 11, 2026

Copy link
Copy Markdown

What this PR does

The TSDB head appender silently drops samples whose timestamp collides with an already-stored sample: exact duplicates (client retries, duplicate shippers, Kafka replay after an ingester restart) and same-timestamp conflicts that are only detectable at commit time. Append returns no error for these, so the ingester counted them as ingested and recorded no discard — making ingestion impossible to reconcile with what senders report.

After a successful commit, the ingester now reads tsdb.CommitStats from the appender (added in grafana/mimir-prometheus#1259) and reclassifies the drops:

  • cortex_discarded_samples_total gains reason same-value-for-timestamp (exact duplicates); commit-time conflicts count into the existing new-value-for-timestamp.
  • cortex_ingester_ingested_samples_total no longer counts the dropped samples (it now reflects what was actually stored).
  • The drops are attributed per series in cost attribution.
  • Dropped duplicates still refresh the TSDB last-update timestamp, so duplicate-only traffic does not idle-close the tenant TSDB.

The stats capability is part of extendedAppender, so an appender wrapper that fails to forward it breaks loudly on the first push instead of silently zeroing the accounting.

Notes for reviewers:

Validated end-to-end on a local ingest-storage cluster and on a classic-architecture dev cell: per-scenario discard accounting, first-value-wins storage, identical counts across zones, and the Kafka-replay restart path.

Which issue(s) this PR fixes or relates to

Fixes #15552

Checklist

Co-authored with @duricanikolic (design + prototype + tests).

🤖 Generated with Claude Code

@cla-assistant

cla-assistant Bot commented Aug 11, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@cla-assistant

cla-assistant Bot commented Aug 11, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

💻 Deploy preview available (Mimir):

dmatth1 and others added 2 commits August 11, 2026 13:35
Brings in the CommitStats appender capability and the
prometheus_tsdb_head_duplicate_samples_dropped_total metric from
grafana/mimir-prometheus#1259.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Dan Mattheiss <dan.mattheiss@grafana.com>
The TSDB head appender silently drops samples whose timestamp collides
with an already-stored sample: exact duplicates (same value, e.g.
client retries, Kafka replay), and same-timestamp conflicts that are
only detectable at commit time (duplicates within one batch, or races
with concurrent appends). Append returns no error for these, so the
ingester counted them as ingested and recorded no discard, making
ingestion impossible to reconcile with what senders report.

After a successful commit, read tsdb.DiscardedSampleStats from the
appender and reclassify the drops:

- cortex_discarded_samples_total gains reason same-value-for-timestamp
  for exact duplicates; commit-time conflicts count into the existing
  new-value-for-timestamp reason.
- cortex_ingester_ingested_samples_total no longer counts the dropped
  samples (it now reflects post-commit truth).
- The drops are attributed per series in cost attribution.
- Dropped duplicates still refresh the TSDB last-update timestamp, so
  duplicate-only traffic does not idle-close the tenant TSDB.

The stats capability is part of extendedAppender, so a future appender
wrapper that fails to forward it breaks loudly on the first push
instead of silently zeroing the accounting.

Co-authored-by: Yuri Nikolic <durica.nikolic@grafana.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Dan Mattheiss <dan.mattheiss@grafana.com>
@dmatth1
dmatth1 force-pushed the dan/ingester-duplicate-sample-stats branch from cdc565f to 945b18f Compare August 11, 2026 19:35
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.

Feature request: ingester should expose a metric for non-appended samples.

1 participant