Add out-of-order sample support #2187

codesome · 2022-06-23T06:27:18Z

What this PR does

This PR adds out of order support to Mimir. #2176 is a prerequisite, and after that is merged, the diff will be smaller after a rebase.

This PR consolidates all the changes from the out-of-order branch (minus vendoring) into a single commit.

Documentation will be added in a separate PR (see this)

I am keeping this as draft until #2176 is merged and this PR is rebased.

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

CHANGELOG.md

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> Co-authored-by: Jesus Vazquez <jesus.vazquez@grafana.com>

CHANGELOG.md

cmd/mimir/help-all.txt.tmpl

pkg/ingester/ingester.go

pstibrany

Thank you. I've left some nit comments, but nothing blocking. I think docs team should take a look at new messages for user (CLI parameters, error messages). (ping @osg-grafana)

pkg/ingester/ingester.go

pkg/ingester/metrics.go

pkg/util/validation/limits.go

CHANGELOG.md

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

codesome · 2022-06-23T14:55:06Z

I have fixed the comments and (1) Renamed the exemplar update flag (2) Calling it OutOfOrderTimeWindow now (and not allowance) and reflected it everywhere including the flags. We will port it to mimir-prometheus in near future. Thanks!

pkg/ingester/ingester_test.go

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

jesusvazquez

Looks good to me 🎉 I'd wait for a maintainer approval though!

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

pkg/ingester/metrics.go

pstibrany

Left final nits. LGTM! Amazing achievement to get OOO support into Mimir! 🎉

docs/sources/operators-guide/mimir-runbooks/_index.md

pkg/ingester/ingester_test.go

pkg/ingester/metrics.go

pkg/util/validation/limits.go

CHANGELOG.md

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

CHANGELOG.md

cmd/mimir/config-descriptor.json

cmd/mimir/help-all.txt.tmpl

docs/sources/operators-guide/configuring/reference-configuration-parameters/index.md

docs/sources/operators-guide/mimir-runbooks/_index.md

pkg/ingester/ingester.go

pkg/ingester/metrics_test.go

pkg/util/validation/limits.go

osg-grafana

Unblocking with more extensive feedback

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

pkg/storage/tsdb/config.go

pkg/util/globalerror/errors.go

pkg/util/validation/limits.go

pkg/ingester/metrics_test.go

pkg/ingester/ingester.go

pracucci · 2022-06-24T13:14:33Z

pkg/ingester/ingester.go

@@ -85,6 +85,7 @@ const (
 	instanceIngestionRateTickInterval = time.Second

 	sampleOutOfOrder     = "sample-out-of-order"
+	sampleTooOld         = "sample-too-old"


I think we're going to have some confusion between sampleOutOfBounds and sampleTooOld. From an user perspective, they're the same: the same timestamp is too old. What if we merge them?

The other error is sample-out-of-bounds or sample-out-of-order. We intentionally changed the error message to have a single on when out of order support is enabled: it is either too old or not too old. What do you suggest here? (the other case was simple where you called out of bound sample as too old)

I see what you mean. Practically, isn't always an "out of bounds" error? Even when you enable the "out of order ingestion" you still have "bounds". I would like to hear @pstibrany opinion too, cause I also see a value in keeping the different cases distinct in metrics. 🤔

I would start with separate metrics so that we can learn more about the feature, but I don't have a strong opinion about it.

Yes, we still have bounds. But the meaning before and after ooo is totally different. "too old" felt more natural. Maybe re-using out-of-bounds is a smarter idea (not too late to make that change btw).

* Add out-of-order sample support Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> Co-authored-by: Jesus Vazquez <jesus.vazquez@grafana.com> * Fix review comments Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> * Fix tests Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> * Update test to check runtime change of OutOfOrderTimeWindow Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> * Fix race in the test Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> * Fix Peter's comments Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> * Fix CI Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> * Fix review comments Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> Co-authored-by: Jesus Vazquez <jesus.vazquez@grafana.com>

codesome mentioned this pull request Jun 23, 2022

Mimir documentation about out-of-order #2183

Merged

codesome force-pushed the codesome/merge-ooo branch 2 times, most recently from 8b98d5d to 656a60e Compare June 23, 2022 06:33

jesusvazquez reviewed Jun 23, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

codesome force-pushed the codesome/merge-ooo branch from 656a60e to bab8154 Compare June 23, 2022 07:46

Add out-of-order sample support

c434389

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> Co-authored-by: Jesus Vazquez <jesus.vazquez@grafana.com>

codesome force-pushed the codesome/merge-ooo branch from bab8154 to c434389 Compare June 23, 2022 08:54

codesome marked this pull request as ready for review June 23, 2022 08:54

codesome requested review from pstibrany and jesusvazquez June 23, 2022 08:54

krajorama mentioned this pull request Jun 23, 2022

[mimir-distributed] Frequently Gaps Inside Mimir Metrics grafana/helm-charts#1517

Open

colega reviewed Jun 23, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

colega reviewed Jun 23, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

colega reviewed Jun 23, 2022

View reviewed changes

cmd/mimir/help-all.txt.tmpl Outdated Show resolved Hide resolved

colega reviewed Jun 23, 2022

View reviewed changes

pkg/ingester/ingester.go Outdated Show resolved Hide resolved

colega reviewed Jun 23, 2022

View reviewed changes

pkg/ingester/ingester.go Outdated Show resolved Hide resolved

pstibrany reviewed Jun 23, 2022

View reviewed changes

Fix review comments

acea9b4

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

pstibrany reviewed Jun 23, 2022

View reviewed changes

pkg/ingester/ingester_test.go Outdated Show resolved Hide resolved

Fix tests

a4517e7

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

jesusvazquez approved these changes Jun 23, 2022

View reviewed changes

codesome force-pushed the codesome/merge-ooo branch 2 times, most recently from afdefc6 to 257d234 Compare June 24, 2022 03:41

Update test to check runtime change of OutOfOrderTimeWindow

8bb0d88

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

codesome force-pushed the codesome/merge-ooo branch from 257d234 to 8bb0d88 Compare June 24, 2022 03:44

Fix race in the test

c204a50

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

codesome commented Jun 24, 2022

View reviewed changes

pkg/ingester/metrics.go Show resolved Hide resolved

pstibrany approved these changes Jun 24, 2022

View reviewed changes

Fix Peter's comments

7a60866

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

codesome force-pushed the codesome/merge-ooo branch from 84bc24f to 7a60866 Compare June 24, 2022 07:36

Fix CI

a0c3ba9

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>