Add RW2 support #11100

krajorama · 2025-04-03T10:37:45Z

What this PR does

Optimized version of #10432 .
Instead of unmarshal into RW2 then convert to RW1, unmarshal directly into RW1.

Which issue(s) this PR fixes or relates to

Related to #9072

Leftovers for next PRs

check if -timeseries-unmarshal-caching-optimization-enabled is possible for RW2
NHCB should be allowed feat(nhcb): add support for NHCB over RW1/RW2 #11143
drop empty labels during unmarshal, see test case in integration/distributor.go "drop empty labels"
implement created timestamp support (RW2)
implement created timestamp support (OTEL)
add metric so we can track rw1 vs rw2 usage

Checklist

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

Add the RW2 proto definition to mimirpb. The implementation will follow but it needs to patch the generated files, so we commit the proto now to have actually readable diff in the PR that has the functionality. See #10423 and especially #11100 diff. Signed-off-by: György Krajcsovits <[email protected]>

CLAassistant · 2025-04-04T08:34:38Z

All committers have signed the CLA.

* feat(rw2): prepare for RW2 support Add the RW2 proto definition to mimirpb. The implementation will follow but it needs to patch the generated files, so we commit the proto now to have actually readable diff in the PR that has the functionality. See #10423 and especially #11100 diff. * test(AddTypeToTree): allow testing for protobuf type and field number Signed-off-by: György Krajcsovits <[email protected]> Co-authored-by: Copilot <[email protected]>

Signed-off-by: György Krajcsovits <[email protected]>

Implement unmarshal of Promtheus remote write request 1.0 data from Remote Write 2.0 input by converting the model. The conversion is lossy as RW1 does not have metadata per series, only per metric family. Signed-off-by: György Krajcsovits <[email protected]> Signed-off-by: Arve Knudsen <[email protected]>

Duplicate all existing testcases with RW2. Add metadata check. Signed-off-by: György Krajcsovits <[email protected]>

Copilot

Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

pkg/util/test/rw2.go:124

The GetSymbols method builds the symbols slice by iterating over a map, which does not guarantee deterministic ordering. Consider iterating sequentially from 0 to the highest index to ensure a consistent order of symbols for downstream consumers.

func (symbols *SymbolTableBuilder) GetSymbols() []string {

pkg/mimirpb/compat_rw2.go:50

[nitpick] After releasing pages to the pool, it may be beneficial to reset the overall pages slice (e.g. setting ps.pages to nil) to avoid retaining memory if the rw2PagedSymbols instance is reused.

page = page[:0]

krajorama · 2025-04-07T10:22:34Z

$ /home/krajo/opt/go/bin/go  test -count 5 -benchtime 5s -benchmem -run=^$ -tags requires_docker,stringlabels -bench ^BenchmarkUnMarshal$ github.com/grafana/mimir/pkg/mimirpb 
goos: linux
goarch: amd64
pkg: github.com/grafana/mimir/pkg/mimirpb
cpu: Intel(R) Core(TM) Ultra 7 155U
BenchmarkUnMarshal/RW1/skipExemplars=true-14         	     219	  27812540 ns/op	57122836 B/op	   60428 allocs/op
BenchmarkUnMarshal/RW1/skipExemplars=true-14         	     219	  26408683 ns/op	57122776 B/op	   60428 allocs/op
BenchmarkUnMarshal/RW1/skipExemplars=true-14         	     225	  26164794 ns/op	57122745 B/op	   60428 allocs/op
BenchmarkUnMarshal/RW1/skipExemplars=true-14         	     220	  26227564 ns/op	57122926 B/op	   60428 allocs/op
BenchmarkUnMarshal/RW1/skipExemplars=true-14         	     228	  26105894 ns/op	57122717 B/op	   60428 allocs/op
BenchmarkUnMarshal/RW2/skipExemplars=true-14         	     206	  29076566 ns/op	32797341 B/op	   50038 allocs/op
BenchmarkUnMarshal/RW2/skipExemplars=true-14         	     204	  27696413 ns/op	32797400 B/op	   50038 allocs/op
BenchmarkUnMarshal/RW2/skipExemplars=true-14         	     212	  28644989 ns/op	32797279 B/op	   50038 allocs/op
BenchmarkUnMarshal/RW2/skipExemplars=true-14         	     216	  28718101 ns/op	32801962 B/op	   50038 allocs/op
BenchmarkUnMarshal/RW2/skipExemplars=true-14         	     217	  28160463 ns/op	32797113 B/op	   50038 allocs/op
BenchmarkUnMarshal/RW1/skipExemplars=false-14        	     202	  29876341 ns/op	61922472 B/op	  100428 allocs/op
BenchmarkUnMarshal/RW1/skipExemplars=false-14        	     201	  30136930 ns/op	61922570 B/op	  100428 allocs/op
BenchmarkUnMarshal/RW1/skipExemplars=false-14        	     200	  31102272 ns/op	61922498 B/op	  100428 allocs/op
BenchmarkUnMarshal/RW1/skipExemplars=false-14        	     192	  30008941 ns/op	61922549 B/op	  100428 allocs/op
BenchmarkUnMarshal/RW1/skipExemplars=false-14        	     198	  29482934 ns/op	61922521 B/op	  100428 allocs/op
BenchmarkUnMarshal/RW2/skipExemplars=false-14        	     192	  30347598 ns/op	34403239 B/op	   60038 allocs/op
BenchmarkUnMarshal/RW2/skipExemplars=false-14        	     204	  29438473 ns/op	34397479 B/op	   60038 allocs/op
BenchmarkUnMarshal/RW2/skipExemplars=false-14        	     202	  29452409 ns/op	34397514 B/op	   60038 allocs/op
BenchmarkUnMarshal/RW2/skipExemplars=false-14        	     202	  30215303 ns/op	34407889 B/op	   60038 allocs/op
BenchmarkUnMarshal/RW2/skipExemplars=false-14        	     200	  31016646 ns/op	34392315 B/op	   60038 allocs/op
PASS
ok  	github.com/grafana/mimir/pkg/mimirpb	178.543s

krajorama · 2025-04-07T10:31:13Z

pkg/mimirpb/timeseries.go

+//   - is symbols is not nil, we unmarshal from Remote Write 2.0 format.
+func (p *PreallocTimeseries) Unmarshal(dAtA []byte, symbols *rw2PagedSymbols, metadata map[string]*MetricMetadata) error {
+	if TimeseriesUnmarshalCachingEnabled && symbols == nil {
+		// TODO(krajorama): check if it makes sense for RW2 as well.


@aknuds1 wdyt?

Moving the new PR Signed-off-by: György Krajcsovits <[email protected]>

aknuds1

Took an initial review pass.

aknuds1 · 2025-04-07T12:57:48Z

pkg/distributor/push.go

+			addWriteResponseStats(w, rs)
+		} else {
+			// This should not happen, but if it does, we should not panic.
+			addWriteResponseStats(w, &promRemote.WriteResponseStats{})


Isn't addWriteResponseStats only relevant for RW v2?

hmm, you're right, I've ended up doing it for RW1 as well, let me see how I could avoid that

fixed, only do stats for RW2 and also add check to the integration test

pkg/distributor/push.go

pkg/mimirpb/mimir.pb.go.expdiff

aknuds1 · 2025-04-07T13:04:12Z

@krajorama Could you compare your new benchmark results with those from main (i.e. benchstats)?

Signed-off-by: György Krajcsovits <[email protected]>

pkg/mimirpb/compat_rw2.go

krajorama · 2025-04-07T14:39:29Z

@krajorama Could you compare your new benchmark results with those from main (i.e. benchstats)?

Main RW1 -> PR RW2

$ benchstat main-rw1.bench pr-rw2.bench 
goos: linux
goarch: amd64
pkg: github.com/grafana/mimir/pkg/mimirpb
cpu: Intel(R) Core(TM) Ultra 7 155U
                                    │ main-rw1.bench │            pr-rw2.bench             │
                                    │     sec/op     │    sec/op     vs base               │
UnMarshal/RW/skipExemplars=true-14       29.52m ± 2%   30.58m ± 29%       ~ (p=0.165 n=10)
UnMarshal/RW/skipExemplars=false-14      33.07m ± 2%   33.06m ±  3%       ~ (p=1.000 n=10)
geomean                                  31.25m        31.80m        +1.76%

                                    │ main-rw1.bench │             pr-rw2.bench             │
                                    │      B/op      │     B/op      vs base                │
UnMarshal/RW/skipExemplars=true-14      54.48Mi ± 0%   31.28Mi ± 0%  -42.58% (p=0.000 n=10)
UnMarshal/RW/skipExemplars=false-14     59.06Mi ± 0%   32.81Mi ± 0%  -44.45% (p=0.000 n=10)
geomean                                 56.72Mi        32.03Mi       -43.52%

                                    │ main-rw1.bench │            pr-rw2.bench             │
                                    │   allocs/op    │  allocs/op   vs base                │
UnMarshal/RW/skipExemplars=true-14       60.43k ± 0%   50.04k ± 0%  -17.19% (p=0.000 n=10)
UnMarshal/RW/skipExemplars=false-14     100.43k ± 0%   60.04k ± 0%  -40.22% (p=0.000 n=10)
geomean                                  77.90k        54.81k       -29.64%

Main RW1 -> PR RW1

benchstat main-rw1.bench pr-rw1.bench 
goos: linux
goarch: amd64
pkg: github.com/grafana/mimir/pkg/mimirpb
cpu: Intel(R) Core(TM) Ultra 7 155U
                                    │ main-rw1.bench │            pr-rw1.bench             │
                                    │     sec/op     │    sec/op     vs base               │
UnMarshal/RW/skipExemplars=true-14       29.52m ± 2%   29.37m ± 31%       ~ (p=0.684 n=10)
UnMarshal/RW/skipExemplars=false-14      33.07m ± 2%   33.37m ±  1%       ~ (p=0.063 n=10)
geomean                                  31.25m        31.31m        +0.20%

                                    │ main-rw1.bench │            pr-rw1.bench             │
                                    │      B/op      │     B/op      vs base               │
UnMarshal/RW/skipExemplars=true-14      54.48Mi ± 0%   54.48Mi ± 0%       ~ (p=0.493 n=10)
UnMarshal/RW/skipExemplars=false-14     59.06Mi ± 0%   59.06Mi ± 0%       ~ (p=1.000 n=10)
geomean                                 56.72Mi        56.72Mi       -0.00%

                                    │ main-rw1.bench │             pr-rw1.bench             │
                                    │   allocs/op    │  allocs/op   vs base                 │
UnMarshal/RW/skipExemplars=true-14       60.43k ± 0%   60.43k ± 0%       ~ (p=1.000 n=10)
UnMarshal/RW/skipExemplars=false-14      100.4k ± 0%   100.4k ± 0%       ~ (p=1.000 n=10) ¹
geomean                                  77.90k        77.90k       +0.00%
¹ all samples are equal

pr-rw2.txt
pr-rw1.txt
main-rw1.txt

pkg/distributor/push.go

pkg/mimirpb/compat_rw2.go

pkg/mimirpb/timeseries.go

pkg/mimirpb/compat_rw2.go

colega

Overall LGTM, I left some nits, please check them.

My main concern isn't regarding this specific implementation but RW2 itself: it seems that we rely on the fact that the symbol table would be sent before the timeseries, but that doesn't seem to be something enforced by protobuf spec, is it?

When a message is serialized, there is no guaranteed order for how its known or unknown fields will be written. Serialization order is an implementation detail, and the details of any particular implementation may change in the future. Therefore, protocol buffer parsers must be able to parse fields in any order.

Signed-off-by: György Krajcsovits <[email protected]>

krajorama · 2025-04-09T08:07:58Z

I've been investigating how the new code interacts with GRPC buffer release/reuse:

While the push is being processed, we don't let go of the request, so GRPC won't do anything with it I assume. After that we call mimirpb.ReuseSlice(req.Timeseries) which will in its inside end up clearing the reference to the yolostrings in label names/values for series and exemplars.

However, for the metadata unit and help string, there's no such cleanup, because RW1 simply copied those.

The problem is that when I process the symbols, I have no idea if the symbol will be used for metadata or for labels, so I just yolostring all. Basically this means I need to make a copy somewhere.

krajorama · 2025-04-09T08:48:31Z

However, for the metadata unit and help string, there's no such cleanup, because RW1 simply copied those.

The problem is that when I process the symbols, I have no idea if the symbol will be used for metadata or for labels, so I just yolostring all. Basically this means I need to make a copy somewhere.

On the other hand we only reuse the WriteRequest.Timeseries not the metadata and the distributor doesn't store metadata, just validate+forward, so it should not be a real problem. We'll see if there some kind of memory leak.

Signed-off-by: György Krajcsovits <[email protected]>

bwplotka · 2025-04-09T09:04:34Z

👋🏽

Disclaimer: I'm not allowed to read Mimir specific code (AGPLv3 license blocks me, sorry), but @krajorama reached me for questions around protobuf and PRWv2 which are super interesting, I might have some pointers.

First of all I love the unmarshalling technique you do here. I would propose we do this in some shared Apache2 shared space, so everyone can maintain and use it e.g. Prometheus or custom Go proto generator we discuss a lot.

I am not sure about the order of fields in the byte stream. Knowing protobuf project, they might be reserving the right to change it if needed, so it might non-deterministic.

To be on safer side, my suggestion is do what we did in Prometheus for the protobuf scrape parsing. We wrote decoder that streams and allow parsing to a concrete type without intermediate structure.

As I understand from the discussion of this PR, you do similar but even more in-place (directly on unmarshal). This allows removing the need to keep some network bytes for a bit longer, but might be prone to the field order. Maybe we can prove you can rely on field order. We could check with Protobuf experts.

Again, whatever you do, this decoder could live on Prometheus or we could play with custom generator for streaming decoding.

Thanks for innovating here 💪🏽

bwplotka · 2025-04-09T09:05:46Z

Just noticed you do this on grpc level, so maybe full decoder is not trivial? Would love to learn more on constraints here.

krajorama · 2025-04-09T09:36:34Z

To be on safer side, my suggestion is do prometheus/prometheus#15731. We wrote decoder that streams and allow parsing to a concrete type without intermediate structure.

These examples are very much remote write 1 and scraped protobuf related as far as I can tell, so they don't suffer from the potential problem of symbols coming later than label/unit/help references.

But I'll mention the potential for problems in the changelog under limitations. I think we can add a slow path that would detect whether symbols was read or not and somehow defer the resolution.

Signed-off-by: György Krajcsovits <[email protected]>

Copilot

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

pkg/util/test/unindent.go:41

[nitpick] Consider enhancing this error message by including the problematic line content to aid debugging of unexpected indentation issues.

t.Fatal("non empty line is shorter than the indentation")

pkg/mimirpb/split_test.go:169

[nitpick] Review the RW2 field names in the expected list; consider aligning the naming conventions (e.g. using consistent capitalization for "SymbolsRW2" and "rw2symbols") to avoid confusion.

assert.ElementsMatch(t, []string{ "Timeseries", "Source", "Metadata", "SymbolsRW2", "TimeseriesRW2", "SkipLabelValidation", "SkipLabelCountValidation", "skipUnmarshalingExemplars", "unmarshalFromRW2", "rw2symbols", }, fieldNames)

Copilot · 2025-04-09T11:26:28Z

pkg/mimirpb/timeseries.go

-// if p.skipUnmarshalingExemplars is false.
-func (p *PreallocTimeseries) Unmarshal(dAtA []byte) error {
-	if TimeseriesUnmarshalCachingEnabled {
+// Copied from the protobuf generated code, change are that:


Correct the comment to read "// Copied from the protobuf generated code, changes are as follows:" to improve clarity.

Suggested change

// Copied from the protobuf generated code, change are that:

// Copied from the protobuf generated code, changes are as follows:

aknuds1

LGTM, but see comments (in particular the error string, which I wonder about).

integration/distributor_test.go

pkg/distributor/push.go

Signed-off-by: György Krajcsovits <[email protected]>

krajorama changed the base branch from main to krajo/wip-rw2 April 3, 2025 10:41

krajorama mentioned this pull request Apr 3, 2025

feat(rw2): prepare for RW2 support #11101

Merged

krajorama force-pushed the krajo/wip-rw2-direct branch from fc8df4d to ee6f843 Compare April 4, 2025 08:34

krajorama changed the base branch from krajo/wip-rw2 to krajo/rw2-proto April 4, 2025 08:34

krajorama mentioned this pull request Apr 4, 2025

POC RW2 to RW1 and new fields #10432

Closed

4 tasks

Base automatically changed from krajo/rw2-proto to main April 4, 2025 14:07

krajorama force-pushed the krajo/wip-rw2-direct branch 2 times, most recently from 2268095 to f697a73 Compare April 4, 2025 14:35

Add unmarshal benchmark

4fc7ccd

Signed-off-by: György Krajcsovits <[email protected]>

krajorama force-pushed the krajo/wip-rw2-direct branch 5 times, most recently from b8f2eb1 to d0df4d9 Compare April 4, 2025 18:44

krajorama force-pushed the krajo/wip-rw2-direct branch from d0df4d9 to 5523a3f Compare April 4, 2025 19:20

test(rw2): enhance distributor integration tests

f029c1d

Duplicate all existing testcases with RW2. Add metadata check. Signed-off-by: György Krajcsovits <[email protected]>

krajorama marked this pull request as ready for review April 5, 2025 20:10

krajorama requested a review from a team as a code owner April 5, 2025 20:10

aknuds1 requested a review from Copilot April 6, 2025 14:17

Copilot AI reviewed Apr 6, 2025

View reviewed changes

krajorama commented Apr 7, 2025

View reviewed changes

remove TODO test

705b352

Moving the new PR Signed-off-by: György Krajcsovits <[email protected]>

aknuds1 reviewed Apr 7, 2025

View reviewed changes

krajorama added 2 commits April 7, 2025 16:01

fix(rw2): fix handling statistics

0ca712c

Signed-off-by: György Krajcsovits <[email protected]>

fix(typo): fix wrong docstring

bbb59f6

Signed-off-by: György Krajcsovits <[email protected]>

krajorama commented Apr 7, 2025

View reviewed changes

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

colega reviewed Apr 8, 2025

View reviewed changes

pkg/distributor/push.go Outdated Show resolved Hide resolved

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

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

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

colega approved these changes Apr 8, 2025

View reviewed changes

krajorama added 3 commits April 9, 2025 08:26

address review comments

264adb1

Signed-off-by: György Krajcsovits <[email protected]>

update expdiff

6da844c

Signed-off-by: György Krajcsovits <[email protected]>

followups

bda00c1

Signed-off-by: György Krajcsovits <[email protected]>

krajorama and others added 2 commits April 9, 2025 10:56

add changelog

e3c3275

Signed-off-by: György Krajcsovits <[email protected]>

Merge branch 'main' into krajo/wip-rw2-direct

cce29a8

update changelog

944ed6e

Signed-off-by: György Krajcsovits <[email protected]>

aknuds1 requested a review from Copilot April 9, 2025 11:25

Copilot AI reviewed Apr 9, 2025

View reviewed changes

aknuds1 approved these changes Apr 9, 2025

View reviewed changes

integration/distributor_test.go Outdated Show resolved Hide resolved

pkg/distributor/push.go Outdated Show resolved Hide resolved

address review comments

747361b

Signed-off-by: György Krajcsovits <[email protected]>

krajorama enabled auto-merge (squash) April 9, 2025 13:04

krajorama merged commit 04a1c19 into main Apr 9, 2025
25 checks passed

krajorama deleted the krajo/wip-rw2-direct branch April 9, 2025 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RW2 support #11100

Add RW2 support #11100

krajorama commented Apr 3, 2025 •

edited

Loading

CLAassistant commented Apr 4, 2025 •

edited

Loading

Copilot AI left a comment

krajorama commented Apr 7, 2025

krajorama Apr 7, 2025

aknuds1 left a comment

aknuds1 Apr 7, 2025

krajorama Apr 7, 2025

krajorama Apr 7, 2025

aknuds1 commented Apr 7, 2025 •

edited

Loading

krajorama commented Apr 7, 2025 •

edited

Loading

colega left a comment

krajorama commented Apr 9, 2025

krajorama commented Apr 9, 2025 •

edited

Loading

bwplotka commented Apr 9, 2025

bwplotka commented Apr 9, 2025

krajorama commented Apr 9, 2025

Copilot AI left a comment

Copilot AI Apr 9, 2025

aknuds1 left a comment

	// Copied from the protobuf generated code, change are that:
	// Copied from the protobuf generated code, changes are as follows:

Add RW2 support #11100

Add RW2 support #11100

Conversation

krajorama commented Apr 3, 2025 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Leftovers for next PRs

Checklist

CLAassistant commented Apr 4, 2025 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

krajorama commented Apr 7, 2025

krajorama Apr 7, 2025

Choose a reason for hiding this comment

aknuds1 left a comment

Choose a reason for hiding this comment

aknuds1 Apr 7, 2025

Choose a reason for hiding this comment

krajorama Apr 7, 2025

Choose a reason for hiding this comment

krajorama Apr 7, 2025

Choose a reason for hiding this comment

aknuds1 commented Apr 7, 2025 • edited Loading

krajorama commented Apr 7, 2025 • edited Loading

colega left a comment

Choose a reason for hiding this comment

krajorama commented Apr 9, 2025

krajorama commented Apr 9, 2025 • edited Loading

bwplotka commented Apr 9, 2025

bwplotka commented Apr 9, 2025

krajorama commented Apr 9, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Apr 9, 2025

Choose a reason for hiding this comment

aknuds1 left a comment

Choose a reason for hiding this comment

krajorama commented Apr 3, 2025 •

edited

Loading

CLAassistant commented Apr 4, 2025 •

edited

Loading

aknuds1 commented Apr 7, 2025 •

edited

Loading

krajorama commented Apr 7, 2025 •

edited

Loading

krajorama commented Apr 9, 2025 •

edited

Loading