Add ephemeral storage to ingester #3922

pstibrany · 2023-01-11T12:50:34Z

What this PR does

This PR adds ephemeral storage to ingester. This storage is first created when write request contains ephemeral series. Ephemeral series from write request are then appended to the ephemeral storage.

This PR does include new metrics, but also reuses some existing metrics: specifically "reasons" in cortex_discarded_samples_total do not distinguish between samples for ephemeral series or "persistent" series, and metrics related to adding samples to appender were reused (so append and commit times include both persistent and ephemeral appenders).

List of new metrics is:

cortex_ingester_ingested_ephemeral_samples_total
cortex_ingester_ingested_ephemeral_samples_failures_total
cortex_ingester_ephemeral_series
~~cortex_ingester_memory_series~~ (existed before)
~~cortex_ingester_memory_users~~ (existed before)
cortex_ingester_ephemeral_series_created_total
cortex_ingester_ephemeral_series_removed_total
cortex_ingester_memory_ephemeral_users

This PR does not address limits or querying of ephemeral storage. This will be focus of subsequent PRs.

Changelog will be added later after the feature is finished.

Which issue(s) this PR fixes or relates to

Part of #3884

Checklist

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

osg-grafana

Doc bit looks fine.

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

replay · 2023-01-11T19:54:29Z

pkg/ingester/ingester.go

+
+		level.Debug(spanlog).Log("event", "got appender for ephemeral series", "ephemeralSeries", len(req.EphemeralTimeseries))
+
+		err = i.pushSamplesToAppender(userID, req.EphemeralTimeseries, ephemeralApp, startAppend, &ephemeralStats, updateFirstPartial, nil, 0, minAppendTimeAvailable, minAppendTime, false)


in the future we might want to consider accepting out-of-order samples here as well, but yeah let's ignore that for now

From a quick look it seems to be that we would need to enable WBL for that, but I may be wrong. I plan to explore that option later, after we have the basics down.

pkg/ingester/ingester.go

pkg/mimirpb/compat.go

pkg/ingester/metrics.go

replay · 2023-01-11T23:17:37Z

pkg/ingester/ingester_test.go

+					cortex_discarded_samples_total{group="",reason="sample-out-of-order",user="test"} 1
+			`,
+		},
+		"request with mix of ephemeral and persistent series, with some good and some bad samples plus some metadata": {


with this helper func which i added in this PR you could generate such mixed persistent/ephemeral requests, but not sure if it's worth it to copy this over:

https://github.com/grafana/mimir/pull/3897/files#diff-16b7662ec0e247fa72bf86b63ec8558a8cea51ac2280baf05a25887578132c44R3667-R3675

It can be used like this:

https://github.com/grafana/mimir/pull/3897/files#diff-16b7662ec0e247fa72bf86b63ec8558a8cea51ac2280baf05a25887578132c44R3144

Nice, when it's in, I will start using it.

replay

Thanks, looks great

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

pkg/ingester/metrics_test.go

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

pracucci · 2023-01-12T10:23:39Z

List of new metrics is:

I would add a CHANGELOG entry and mention metrics too.

Edit: I didn't read the next line "Changelog will be added later after the feature is finished.". LGTM.

pracucci

Solid work! I left few comment and questions. Thanks!

pracucci · 2023-01-12T10:25:40Z

docs/sources/mimir/reference-configuration-parameters/index.md

+# CLI flag: -ingester.ephemeral-series-retention-period
+[ephemeral_series_retention_period: <duration> | default = 10m]


I would have expected to see this close to -blocks-storage.tsdb.retention-period. Have you considered having something like -blocks-storage.ephemeral-tsdb.retention-period?

Funny that you ask. I originally started with similar flag, but it then found metadata retention and copied that instead. I don't have strong opinion about naming, I will switch to -blocks-storage.ephemeral-tsdb.retention-period.

pracucci · 2023-01-12T10:26:49Z

pkg/ingester/ingester.go

@@ -306,6 +312,11 @@ func New(cfg Config, limits *validation.Overrides, activeGroupsCleanupService *u
 			Help: "The current number of series in memory.",
 		}, i.getMemorySeriesMetric)

+		promauto.With(registerer).NewGaugeFunc(prometheus.GaugeOpts{
+			Name: "cortex_ingester_ephemeral_series",


[question] Have you considered adding a memory_ to clarify they're in-memory? Similar to cortex_ingester_memory_series .

Suggested change

Name: "cortex_ingester_ephemeral_series",

Name: "cortex_ingester_memory_ephemeral_series",

pracucci · 2023-01-12T10:28:39Z

pkg/ingester/ingester.go

@@ -701,7 +712,7 @@ func (i *Ingester) PushWithCleanup(ctx context.Context, pushReq *push.Request) (

 		// Keep track of some stats which are tracked only if the samples will be
 		// successfully committed
-		stats pushStats
+		stats, ephemeralStats pushStats


[nit] What if we rename stats to persistentStats? Is that how we're going to call the standard TSDB storage, right?

Sounds good. Yes, we use "persistent storage" to refer to original TSDB-based storage.

pracucci · 2023-01-12T10:29:37Z

pkg/ingester/ingester.go

+	rollback := func() {
+		if persistentApp != nil {
+			if err := persistentApp.Rollback(); err != nil {
+				level.Warn(i.logger).Log("msg", "failed to rollback on error", "user", userID, "err", err)


[nit] I would clarify it.

Suggested change

level.Warn(i.logger).Log("msg", "failed to rollback on error", "user", userID, "err", err)

level.Warn(i.logger).Log("msg", "failed to rollback persistent appender on error", "user", userID, "err", err)

pracucci · 2023-01-12T10:29:51Z

pkg/ingester/ingester.go

+	if len(req.Timeseries) > 0 {
+		persistentApp = db.Appender(ctx).(extendedAppender)
+
+		level.Debug(spanlog).Log("event", "got appender", "series", len(req.Timeseries))


Suggested change

level.Debug(spanlog).Log("event", "got appender", "series", len(req.Timeseries))

level.Debug(spanlog).Log("event", "got persistent appender", "series", len(req.Timeseries))

pracucci · 2023-01-12T10:35:49Z

pkg/ingester/ingester.go

-		return nil, wrapWithUser(err, userID)
+	if persistentApp != nil {
+		if err := persistentApp.Commit(); err != nil {
+			rollback()


We'll try to rollback persistentApp too (after Commit() was called and failed). Is it safe?

Documentation for Commit() says:

If Commit returns a non-nil error, it also rolls back all modifications made in the appender so far, as Rollback would do.
In any case, an Appender must not be used anymore after Commit has been called.

Which tells me that 1) Rollback is automatic in case of errors, 2) we should not use the appender. Let's clear it here. Same for ephemeralApp below.

pracucci · 2023-01-12T10:38:43Z

pkg/ingester/ingester.go

+	db.updatedRatesFromStats(stats.succeededSamplesCount, req.Source)
+	db.updatedRatesFromStats(ephemeralStats.succeededSamplesCount, req.Source)


Why do we need this new function and can't keep the metrics update in updateMetricsFromPushStats(), passing the req.Source there?

We also need to pass db there. We can do that too.

pracucci · 2023-01-12T10:40:02Z

pkg/ingester/ingester.go

+			IsolationDisabled:              true,
+		}
+
+		// We need to set this, despite OOO time window being 0.


Can you explain why we need to set it to i.cfg.BlocksStorageConfig.TSDB.OutOfOrderCapacityMax?

We need some value, because Head checks that it's set, but it won't be used as we don't enable OOO. I used i.cfg.BlocksStorageConfig.TSDB.OutOfOrderCapacityMax to get some valid value. I will switch it to tsdb.DefaultOutOfOrderCapMax.

pracucci · 2023-01-12T10:43:06Z

pkg/ingester/ingester.go

+		// We need to set this, despite OOO time window being 0.
+		headOptions.OutOfOrderCapMax.Store(int64(i.cfg.BlocksStorageConfig.TSDB.OutOfOrderCapacityMax))
+
+		h, err := tsdb.NewHead(prometheus.WrapRegistererWithPrefix(ephemeralPrometheusMetricsPrefix, tsdbPromReg), log.With(userLogger, "ephemeral", "true"), nil, nil, headOptions, nil)


[question] Have you considered "storage=ephemeral" instead? And then wrap the logger passed to tsdb.Open() with storage=persistent?

Suggested change

h, err := tsdb.NewHead(prometheus.WrapRegistererWithPrefix(ephemeralPrometheusMetricsPrefix, tsdbPromReg), log.With(userLogger, "ephemeral", "true"), nil, nil, headOptions, nil)

h, err := tsdb.NewHead(prometheus.WrapRegistererWithPrefix(ephemeralPrometheusMetricsPrefix, tsdbPromReg), log.With(userLogger, "storage", "ephemeral"), nil, nil, headOptions, nil)

pracucci · 2023-01-12T10:45:01Z

pkg/ingester/ingester.go

@@ -2011,7 +2126,7 @@ func (i *Ingester) compactBlocks(ctx context.Context, force bool, allowed *util.

 		default:
 			reason = "regular"
-			err = userDB.Compact()
+			err = userDB.Compact(time.Now().Add(-i.cfg.EphemeralSeriesRetentionPeriod))


Have you considered storing EphemeralSeriesRetentionPeriod in userTSDB instead? It's a bit weird reading the function call here. It makes you think it just compacts the ephemeral TSDB, while it compacts both.

pracucci · 2023-01-12T10:49:17Z

From PR description:

cortex_ingester_memory_series

I think you meant cortex_ingester_ephemeral_series, right?

cortex_ingester_memory_users

This is not new, but I can't find the ephemeral counterpart. Where is it defined?

pstibrany · 2023-01-12T13:42:23Z

cortex_ingester_memory_series

I think you meant cortex_ingester_ephemeral_series, right?

Yes, that is already on the list.

cortex_ingester_memory_users

This is not new, but I can't find the ephemeral counterpart. Where is it defined?

There is no ephemeral counterpart for number of users, but I think it's a good idea to add it. Let me to that in this PR.

(I mistakenly included cortex_ingester_memory_series and cortex_ingester_memory_users on the list, because I copied the list of metrics from the test. I updated the description)

…reuse values from TSDBConfig. Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

pstibrany · 2023-01-12T15:03:25Z

Thank you both for your reviews! I believe I've addressed all your feedback now.

pstibrany · 2023-01-12T15:28:32Z

Thank you both for your reviews! I believe I've addressed all your feedback now.

I forgot to add this part. Will do that tomorrow.

This is not new, but I can't find the ephemeral counterpart. Where is it defined?

There is no ephemeral counterpart for number of users, but I think it's a good idea to add it. Let me to that in this PR.

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

pstibrany · 2023-01-12T16:09:47Z

This is not new, but I can't find the ephemeral counterpart. Where is it defined?

There is no ephemeral counterpart for number of users, but I think it's a good idea to add it. Let me to that in this PR.

I've added this, it's cortex_ingester_memory_ephemeral_users metric.

pkg/ingester/ingester.go

pkg/ingester/metrics.go

pkg/ingester/user_tsdb.go

replay

I have re-read it, thanks for addressing all the feedback

… series are ingested. Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

pstibrany · 2023-01-13T08:28:25Z

I have re-read it, thanks for addressing all the feedback

Thanks Mauro, I've addressed your latest feedback. I've also added new test TestIngesterTruncationOfEphemeralSeries. I will extend all tests with querying after that part is added in next PRs.

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

pracucci

Excellent job! LGTM 👏

pracucci · 2023-01-13T13:48:51Z

pkg/ingester/ingester.go

+		i.metrics.memEphemeralUsers.Inc()
+
+		// Don't allow ingestion of old samples into ephemeral storage.
+		h.SetMinValidTime(time.Now().Add(-i.cfg.BlocksStorageConfig.EphemeralTSDB.Retention).UnixMilli())


[non blocking, but I would like to talk more about this] Is this actually correct? What if a customer sends samples with an older timestamp (e.g. while catching up after an outage)? Also we should be consistent with any check done in Push(): we allow any timestamp there (up to TSDB out-of-bounds limits).

What if a customer sends samples with an older timestamp (e.g. while catching up after an outage)?

Yes, that's unfortunately how "ephemeral" storage will work.

Also we should be consistent with any check done in Push(): we allow any timestamp there (up to TSDB out-of-bounds limits).

We are consistent, because we're updating minValidTime of ephemeral storage periodically via *userDB.Compact where we call Truncate on ephemeral storage.

pstibrany requested a review from a team as a code owner January 11, 2023 12:50

pstibrany force-pushed the add-ephemeral-storage-to-ingesters branch from 9b2dfff to e36736e Compare January 11, 2023 12:52

pstibrany requested a review from a team as a code owner January 11, 2023 13:47

pstibrany mentioned this pull request Jan 11, 2023

Ephemeral storage #3884

Closed

osg-grafana approved these changes Jan 11, 2023

View reviewed changes

pstibrany added 7 commits January 11, 2023 16:12

Add ephemeral storage to ingester.

bc9465c

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Add basic metrics.

d592209

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Test.

dfc44a8

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Fix const name and value.

bbbcced

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Updated doc.

eb9b1d6

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Fix metrics tests.

01e5339

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

make reference-help

587aaeb

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

pstibrany force-pushed the add-ephemeral-storage-to-ingesters branch from 2c4584e to 587aaeb Compare January 11, 2023 15:12

pstibrany requested review from replay and pracucci January 11, 2023 15:14

replay reviewed Jan 11, 2023

View reviewed changes

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

replay reviewed Jan 11, 2023

View reviewed changes

pkg/mimirpb/compat.go Outdated Show resolved Hide resolved

replay reviewed Jan 11, 2023

View reviewed changes

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

replay reviewed Jan 11, 2023

View reviewed changes

replay approved these changes Jan 11, 2023

View reviewed changes

pstibrany added 2 commits January 12, 2023 10:18

Pass request source to updatedRatesFromStats method.

2292672

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Review feedback.

d9f55b0

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

replay reviewed Jan 12, 2023

View reviewed changes

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

Fix metric help strings.

37c343b

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

pracucci reviewed Jan 12, 2023

View reviewed changes

Move configuration for ephemeral storage into separate struct, don't …

1048a2a

…reuse values from TSDBConfig. Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

pstibrany added 2 commits January 12, 2023 16:01

Address review feedback.

c0b7c5c

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Update help and doc.

4490ca6

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Add cortex_ingester_memory_ephemeral_users metric.

1efb6e1

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

replay reviewed Jan 12, 2023

View reviewed changes

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

replay reviewed Jan 12, 2023

View reviewed changes

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

replay reviewed Jan 12, 2023

View reviewed changes

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

replay approved these changes Jan 12, 2023

View reviewed changes

pstibrany added 3 commits January 13, 2023 09:08

Test that we don't touch metrics for ephemeral series if no ephemeral…

ac78585

… series are ingested. Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

More review feedback.

7718572

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Add test for truncation of ephemeral series.

365d957

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

Fix tests.

5f67937

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

pracucci approved these changes Jan 13, 2023

View reviewed changes

pstibrany merged commit 957fe43 into main Jan 16, 2023

pstibrany deleted the add-ephemeral-storage-to-ingesters branch January 16, 2023 11:01

pstibrany mentioned this pull request Jan 16, 2023

Fix main #3963

Merged

1 task


		level.Debug(spanlog).Log("event", "got appender for ephemeral series", "ephemeralSeries", len(req.EphemeralTimeseries))

		err = i.pushSamplesToAppender(userID, req.EphemeralTimeseries, ephemeralApp, startAppend, &ephemeralStats, updateFirstPartial, nil, 0, minAppendTimeAvailable, minAppendTime, false)

		# CLI flag: -ingester.ephemeral-series-retention-period
		[ephemeral_series_retention_period: <duration> \| default = 10m]

	Name: "cortex_ingester_ephemeral_series",
	Name: "cortex_ingester_memory_ephemeral_series",

	level.Warn(i.logger).Log("msg", "failed to rollback on error", "user", userID, "err", err)
	level.Warn(i.logger).Log("msg", "failed to rollback persistent appender on error", "user", userID, "err", err)

	level.Debug(spanlog).Log("event", "got appender", "series", len(req.Timeseries))
	level.Debug(spanlog).Log("event", "got persistent appender", "series", len(req.Timeseries))

		db.updatedRatesFromStats(stats.succeededSamplesCount, req.Source)
		db.updatedRatesFromStats(ephemeralStats.succeededSamplesCount, req.Source)

	h, err := tsdb.NewHead(prometheus.WrapRegistererWithPrefix(ephemeralPrometheusMetricsPrefix, tsdbPromReg), log.With(userLogger, "ephemeral", "true"), nil, nil, headOptions, nil)
	h, err := tsdb.NewHead(prometheus.WrapRegistererWithPrefix(ephemeralPrometheusMetricsPrefix, tsdbPromReg), log.With(userLogger, "storage", "ephemeral"), nil, nil, headOptions, nil)

Add ephemeral storage to ingester #3922

Add ephemeral storage to ingester #3922

Conversation

pstibrany commented Jan 11, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

osg-grafana left a comment

Choose a reason for hiding this comment

replay Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay left a comment

Choose a reason for hiding this comment

pracucci commented Jan 12, 2023 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci commented Jan 12, 2023

pstibrany commented Jan 12, 2023 • edited Loading

pstibrany commented Jan 12, 2023

pstibrany commented Jan 12, 2023

pstibrany commented Jan 12, 2023

replay left a comment

Choose a reason for hiding this comment

pstibrany commented Jan 13, 2023

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany commented Jan 11, 2023 •

edited

Loading

replay Jan 11, 2023 •

edited

Loading

replay Jan 11, 2023 •

edited

Loading

pracucci commented Jan 12, 2023 •

edited

Loading

pstibrany commented Jan 12, 2023 •

edited

Loading