write path: handle created timestamp #11977

krajorama · 2025-07-04T08:19:06Z

What this PR does

Modify the ingester and block builder to accept and pass the Created timestamp field from incoming write requests to TSDB.

Note1: if samples aren't ordered by time in a remote write requests, then the result is undefined.

Note2: as with regular samples, Mimir will have non-deterministic behavior if there's a sample and a created timestamp with the same timestamp ingested on two different ingesters for whatever reason. During query and block building the resolution of such conflicting samples has always been non-deterministic, as there is no way to know at that point which sample is the correct one.

Which issue(s) this PR fixes or relates to

Fixes #11976

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]. If changelog entry is not needed, please add the changelog-not-needed label to the PR.
about-versioning.md updated with experimental features.

github-actions · 2025-07-04T17:41:40Z

💻 Deploy preview available:

CHANGELOG.md

pkg/blockbuilder/tsdb.go

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Co-authored-by: Taylor C <41653732+tacole02@users.noreply.github.com>

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama · 2025-07-08T19:18:39Z

@tacole02 Thank you for the suggested changes, I've made a small tweak on top. PTAL if you have time, thanks.

jesusvazquez

LGTM the test coverage is great so I think we're good.

I was wondering if it would be possible to have less code duplication between how the ingester does the CT appends and how the blockbuilder does it but i dont think we can do it without making this PR much larger and even then I don't know if it would be worth it.

Great work Krajo

pkg/blockbuilder/tsdb_test.go

narqo · 2025-07-09T10:50:15Z

pkg/blockbuilder/tsdb.go

+				if err != nil && !errors.Is(err, storage.ErrOutOfOrderCT) && !errors.Is(err, storage.ErrOutOfOrderSample) {
+					level.Warn(b.logger).Log("msg", "failed to store zero float sample for created timestamp", "tenant", userID, "err", err)
+				}


I've noticed that the code in ingester, handles this error as "soft-error", ie. errProcessor.ProcessErr(err, ...). I believe the result of that is that the sample will be counted towards discarded samples metric.

This is a nit, but do we want to count a discarded created sample in the block-builder as well?

Also, I've noticed that the logic about skipping the sample when its created timestamp fails is different between the ingester and the block-builder (ref the comment below).

This is a nit, but do we want to count a discarded created sample in the block-builder as well?

Just add discardedSamples++ ?

Yes, just discardedSamples will do that

done, added test as well

narqo · 2025-07-09T10:52:16Z

pkg/ingester/ingester.go

+					stats.succeededSamplesCount++
+				} else if !errors.Is(err, storage.ErrOutOfOrderCT) && !errors.Is(err, storage.ErrOutOfOrderSample) {
+					errProcessor.ProcessErr(err, ts.CreatedTimestamp, ts.Labels)
+					continue


To double-check, the continue here will cause the sample s to be skipped, if there was an error in the append of its created sample. I think, the piece of code in the block-builder doesn't skip the sample. Is this on purpose?

no, I think that's a fair point, shouldn't skip

I've copy pasted the error handling, I'll try to add a unit test to cover this case.

I've modified the code and added tests, but the native histogram one fails as there's a bug in upstream Prometheus. So I need to fix the bug there, and update mimir-prometheus.

Actually the bug is not new prometheus/prometheus#16332

narqo · 2025-07-09T10:59:24Z

I was wondering if it would be possible to have less code duplication between how the ingester does the CT appends and how the blockbuilder does it but i dont think we can do it without making this PR much larger and even then I don't know if it would be worth it.

I did a couple of attempts trying to unify these pieces of the ingester and the block-builder, but that, indeed, resulted in a fairly large refactoring of ingester's internals, which was a bit scary. So every time, I gave up. I still want to do that. Although, tbf, an extra doubt I have currently, comes from the parquet exploration, that folks do in parallel. If parquet is the way, I'm not sure the block-builder will keep building tsdb blocks at all.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

narqo

Besides that we need the fix in the upstream prometheus (ref #11977 (comment)), the changes here look good to me 🔥

tacole02

Looks great! Thanks, @krajorama !

zenador · 2025-07-10T11:26:35Z

🤖 Automated comment

The CHANGELOG has just been cut to prepare for the next release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG.md document. Thanks!

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama · 2025-07-10T14:14:39Z

Looks great! Thanks, @krajorama !

Thanks @tacole02

Unfortunately I have to drop the changes to the existing CHANGELOG entry as the release has been cut in the meantime :( So I'm moving the text to the unreleased section as enhancement.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

#11953 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

tacole02

Looks good!

krajorama force-pushed the krajo/created-timestamp branch 4 times, most recently from 24c0591 to b9e1a00 Compare July 4, 2025 11:33

krajorama changed the title ~~ingester: handle created timestamp~~ write path: handle created timestamp Jul 4, 2025

krajorama force-pushed the krajo/created-timestamp branch from b9e1a00 to 1efc74a Compare July 4, 2025 11:39

krajorama force-pushed the krajo/created-timestamp branch 4 times, most recently from 36c90aa to 7774cee Compare July 5, 2025 10:39

krajorama marked this pull request as ready for review July 5, 2025 10:39

krajorama requested a review from a team as a code owner July 5, 2025 10:39

krajorama marked this pull request as draft July 5, 2025 11:20

krajorama force-pushed the krajo/created-timestamp branch 3 times, most recently from 44740eb to c7ebe12 Compare July 5, 2025 11:58

krajorama marked this pull request as ready for review July 5, 2025 11:58

tacole02 approved these changes Jul 7, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

narqo reviewed Jul 8, 2025

View reviewed changes

pkg/blockbuilder/tsdb.go Outdated Show resolved Hide resolved

krajorama added 3 commits July 8, 2025 21:00

ingester: handle created timestamp

d9a00e3

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

block-builder: handle created timestamp

f304041

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Add e2e test to check that created timestamp is actually processed

eb329d8

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama force-pushed the krajo/created-timestamp branch from c7ebe12 to eb329d8 Compare July 8, 2025 19:10

krajorama and others added 2 commits July 8, 2025 21:16

Apply suggestions from code review

16b0e7b

Co-authored-by: Taylor C <41653732+tacole02@users.noreply.github.com>

Further refine changelog

e3d3dc0

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama requested a review from tacole02 July 8, 2025 19:18

jesusvazquez approved these changes Jul 9, 2025

View reviewed changes

narqo reviewed Jul 9, 2025

View reviewed changes

fix inconsistent error handling between ingester and block builder

22e60cc

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

This was referenced Jul 9, 2025

fix: unlock of unlocked mutex prometheus/prometheus#16332

Merged

chore(tsdb): add a few more testcases for unlock of unlocked mtx 16332 prometheus/prometheus#16848

Merged

count discarded samples in blockbuilder + test

25f6b10

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

narqo approved these changes Jul 9, 2025

View reviewed changes

tacole02 approved these changes Jul 9, 2025

View reviewed changes

zenador added the release/notified-changelog-cut label Jul 10, 2025

krajorama added 2 commits July 10, 2025 16:01

Merge branch 'main' into krajo/created-timestamp

a036bb9

Move changelog due to release being cut

c967a7c

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama added 2 commits July 10, 2025 16:25

Update changelog once more

15f32b9

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Follow-up to 11953

073c583

#11953 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

tacole02 approved these changes Jul 10, 2025

View reviewed changes

krajorama merged commit 6178f37 into main Jul 10, 2025
31 checks passed

krajorama deleted the krajo/created-timestamp branch July 10, 2025 16:27

write path: handle created timestamp #11977

write path: handle created timestamp #11977

Uh oh!

Conversation

krajorama commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

Uh oh!

github-actions bot commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krajorama commented Jul 8, 2025

Uh oh!

jesusvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

narqo commented Jul 9, 2025

Uh oh!

narqo left a comment

Choose a reason for hiding this comment

Uh oh!

tacole02 left a comment

Choose a reason for hiding this comment

Uh oh!

zenador commented Jul 10, 2025

Uh oh!

krajorama commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacole02 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

krajorama commented Jul 4, 2025 •

edited

Loading

github-actions bot commented Jul 4, 2025 •

edited

Loading

krajorama commented Jul 10, 2025 •

edited

Loading