[RFC] Combine Appender.Add and AddFast into a single Append method.

[tomwilkie]

This moves the label lookup into TSDB, whilst still keeping the cached-ref optimisation for repeated Appends. Effectively we merge the two function into one that is given both the labels and any cached ref. If the cached ref is valid, that is used - otherwise a (potentially new) series and ref is found/created using the labels.

This makes the API easier to consume and implement. In particular this change is motivated by the scrape-time-aggregation work, which I don't think is possible to implement without it as it needs access to label values.

I don't believe this should have any performance impact, and am running benchmarks to confirm right now.

Signed-off-by: Tom Wilkie [email protected]

[tomwilkie]

Yeah, no impact on benchmarks - 4.49s vs 4.91s is within the noise for my computer.

➜  prometheus git:(append) ✗ git checkout master                                                      
Switched to branch 'master'
Your branch is up to date with 'origin/master'.                                        
➜  prometheus git:(master) ✗ go build ./cmd/promtool
➜  prometheus git:(master) ✗ ./promtool tsdb bench write --metrics=100000 tsdb/testdata/20kseries.json
level=info ts=2021-02-15T17:04:00.714396Z caller=head.go:668 msg="Replaying on-disk memory mappable chunks if any"
level=info ts=2021-02-15T17:04:00.71446Z caller=head.go:682 msg="On-disk memory mappable chunks replay completed" duration=5.395µs
level=info ts=2021-02-15T17:04:00.714467Z caller=head.go:688 msg="Replaying WAL, this may take a while"
level=info ts=2021-02-15T17:04:00.715027Z caller=head.go:740 msg="WAL segment loaded" segment=0 maxSegment=0
level=info ts=2021-02-15T17:04:00.715057Z caller=head.go:745 msg="WAL replay completed" checkpoint_replay_duration=32.219µs wal_replay_duration=551.153µs total_replay_duration=596.712µs
level=info ts=2021-02-15T17:04:00.716098Z caller=db.go:1388 msg="Compactions disabled"
>> start stage=readData
>> completed stage=readData duration=137.814505ms
>> start stage=ingestScrapes
ingestion completed
>> completed stage=ingestScrapes duration=4.492824812s
 > total samples: 60000000
 > samples/sec: 1.3354603087996313e+07
>> start stage=stopStorage
>> completed stage=stopStorage duration=137.387856ms

➜  prometheus git:(master) ✗ git checkout append                                                      
Switched to branch 'append'
➜  prometheus git:(append) ✗ go build ./cmd/promtool                                                  
➜  prometheus git:(append) ✗ ./promtool tsdb bench write --metrics=100000 tsdb/testdata/20kseries.json
level=info ts=2021-02-15T17:03:20.521251Z caller=head.go:668 msg="Replaying on-disk memory mappable chunks if any"
level=info ts=2021-02-15T17:03:20.521312Z caller=head.go:682 msg="On-disk memory mappable chunks replay completed" duration=4.531µs
level=info ts=2021-02-15T17:03:20.521319Z caller=head.go:688 msg="Replaying WAL, this may take a while"
level=info ts=2021-02-15T17:03:20.521667Z caller=head.go:740 msg="WAL segment loaded" segment=0 maxSegment=0
level=info ts=2021-02-15T17:03:20.521683Z caller=head.go:745 msg="WAL replay completed" checkpoint_replay_duration=32.636µs wal_replay_duration=326.064µs total_replay_duration=374.353µs
level=info ts=2021-02-15T17:03:20.5227Z caller=db.go:1388 msg="Compactions disabled"
>> start stage=readData
>> completed stage=readData duration=141.07832ms
>> start stage=ingestScrapes
ingestion completed
>> completed stage=ingestScrapes duration=4.915914142s
 > total samples: 60000000
 > samples/sec: 1.2205240251674764e+07
>> start stage=stopStorage
>> completed stage=stopStorage duration=123.654169ms

[roidelapluie]

/prombench master

[prombot]

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-8489 and master

After successful deployment, the benchmarking metrics can be viewed at:

• Prometheus Meta
• Prombench Dashboard
• Grafana Explorer, Loki logs

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart master

[roidelapluie]

Thanks! I think it makes sense as Add was calling AddFast anyway.

cc @bwplotka as another consumer of the TSDB

[roidelapluie]

/prombench cancel

Prombench is OK.

[prombot]

Benchmark cancel is in progress.

[codesome]

Thanks!

[pracucci]

This change will introduce a major performance penalty in Cortex ingesters. The way it works in cortex is:

• Labels are YOLO strings
• We lookup our ref cache using series labels as YOLO
• If the series is in the ref cache, we call AddFast(). However, it could fail. If it fail we do a copy of labels and call Add().

After this change, we don't know if labels will be retained or not by Append() so we'll always have to copy it. This will have a major performance penalty in the ingesters.

[codesome]

I am happy to revert this back for now and reconsider how can it be merged into a single method without hurting Cortex perf.

[pracucci]

To give more context, when we introduced the ref cache in Cortex ingesters (using labels as YOLO strings to lookup the cache), memory allocations were reduced by -25%:
cortexproject/cortex#2057

[codesome]

PR to revert this #8519

[tomwilkie]

We lookup our ref cache using series labels as YOLO

Can you store another copy of the labels in the ref cache? This is effectively what Prometheus does (store a copy of the labels in the scape cache, and use them on subsequent calls to Add).

[pracucci]

Can you store another copy of the labels in the ref cache? This is effectively what Prometheus does (store a copy of the labels in the scape cache, and use them on subsequent calls to Add).

Good point. Yes, we should be able to solve it this way.

[tomwilkie]

Let me know if this works! If not we can find another way. I'd note that if you know the ref, you can pass null for the labels... but this is abusing the code a bit...

[bwplotka]

Yes, this is impacting Thanos a bit as well, I believe this PR could be done more carefully instead of merging so quickly. Let me schedule some experiment with this on Thanos side.

[bboreham]

I hadn't noticed this change, but came up with an idea to remove the refcache - see #8600 and cortexproject/cortex#3951

[bboreham]

Am I right in thinking the only way a reference can become invalid is via head compaction?

[codesome]

Am I right in thinking the only way a reference can become invalid is via head compaction?

Yes

[shichanglin5]

hello, I have a question, why don't reuse 'SeriesRef' returned by 'app.Append'? If the 'SeriesRef' invalid (via compactHead gc), will a new memSeries be created?

[bboreham]

Each time this line is entered, labels is different. We can’t reuse the value from the previous loop iteration, and there is nowhere convenient to store it for longer.

[shichanglin5]

	for _, ts := range req.Timeseries {
		labels := labelProtosToLabels(ts.Labels)
		// default to 0，need to search `memSeries` by labels at first traverse
		var ref storage.SeriesRef
		// this inner for loop, labels are always the same
		// this `ts` may have multiple samples, `ref` can be reused to avoid searching `memSeries` each traverse
		for _, s := range ts.Samples {
			ref, err = app.Append(ref, labels, s.Timestamp, s.Value)
			if err != nil {
				switch errors.Cause(err) {
				case storage.ErrOutOfOrderSample, storage.ErrOutOfBounds, storage.ErrDuplicateSampleForTimestamp:
					level.Error(h.logger).Log("msg", "Out of order sample from remote write", "err", err.Error(), "series", labels.String(), "timestamp", s.Timestamp)
				}
				return err
			}
		}

will there be any problem? multiple samples of a time series have the same labels

[bboreham]

Oh I see what you mean.
It’s not typical to send multiple samples for the same timeseries, but your version is more efficient if it did happen.

[shichanglin5]

In this scenario, time series data is generated by aggregating logs and written to the prometheus through remote write, so there will be multiple samples;
I grabbed the cpu profile through pprof, and it looks like labelProtosToLabels and seriesHashmap account for about 7.61 percent of the total. If reuse is SeriesRefit looks like seriesHashmap account for about 3.51 percent of the total. If reuse is SeriesRef, it should be much better. Can I submit a pr to optimize this code?

[shichanglin5]

#12580

[bboreham]

it looks like labelProtosToLabels and seriesHashmap account for about 7.61 percent of the total. If reuse is SeriesRef, it should be much better.

I don't see where labelProtosToLabels would change.

I will note that remote-write handling has been optimised in various downstream projects (Cortex, Thanos, Mimir), using many tricks to avoid allocating memory.