Skip to content

[PRW 2.0] Merging remote-write-2.0 feature branch to main (PRW 2.0 support + metadata in WAL)#14395

Merged
cstyan merged 142 commits intomainfrom
remote-write-2.0-main
Jul 4, 2024
Merged

[PRW 2.0] Merging remote-write-2.0 feature branch to main (PRW 2.0 support + metadata in WAL)#14395
cstyan merged 142 commits intomainfrom
remote-write-2.0-main

Conversation

@bwplotka
Copy link
Copy Markdown
Member

@bwplotka bwplotka commented Jul 2, 2024 •
edited
Loading

This is a merge of remote-write-2.0 feature branch (used copy so we can add extra commits for mergability).

From high level this adds support for Remote Write 2.0, as well metadata in WAL.

Kudos to all who contributed to this, mainly @cstyan @npazosmendez @alexgreenbank and @tpaschalis 🤗

Every commit was already reviewed (until and including @bwplotka Merge pull request #14347 from prometheus/rw20-histogram-helpers
f09c645).

npazosmendez and others added 30 commits November 8, 2023 11:21
@npazosmendez @cstyan
Remote Write 1.1: e2e benchmarks (#13102)
07e21cb 
* Remote Write e2e benchmarks

Signed-off-by: Nicolás Pazos <[email protected]>

* Prometheus ports automatically assigned

Signed-off-by: Nicolás Pazos <[email protected]>

* make dashboard editable + more modular to different job label values

Signed-off-by: Callum Styan <[email protected]>

* Dashboard improvements

* memory stats
* diffs look at counter increases

Signed-off-by: Nicolás Pazos <[email protected]>

* run script: absolute path for config templates

Signed-off-by: Nicolás Pazos <[email protected]>

* grafana dashboard improvements

* show actual values of metrics
* add memory stats and diff

Signed-off-by: Nicolás Pazos <[email protected]>

* dashboard changes

Signed-off-by: Callum Styan <[email protected]>

---------

Signed-off-by: Nicolás Pazos <[email protected]>
Signed-off-by: Callum Styan <[email protected]>
Co-authored-by: Callum Styan <[email protected]>
@cstyan
Merge branch 'main' into remote-write-1.1
b36ea85
@cstyan @npazosmendez
replace snappy encoding library
4c4b9aa 
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Nicolás Pazos <[email protected]>
@cstyan @npazosmendez
add new proto types
1ac2950 
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Nicolás Pazos <[email protected]>
@cstyan @npazosmendez
add decode function for new write request proto
8194000 
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Nicolás Pazos <[email protected]>
@cstyan @npazosmendez
add lookup table struct that is used to build the symbol table in new
0768e55 
write request format

Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Nicolás Pazos <[email protected]>
@cstyan @npazosmendez
Implement code paths for new proto format
ce1e2ad 
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Nicolás Pazos <[email protected]>
@cstyan @npazosmendez
update example server to include handler for new format
b7e3665 
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Nicolás Pazos <[email protected]>
@cstyan @npazosmendez
Add new test client
91bdd93 
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
tests and new -> original proto mapping util
005ba7a 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
add new proto support on receiver end
ab7c96a 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
Fix test
7f7cf97 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
no-brainer copypaste but more performance write support
5f5272e 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
remove some comented code
12de4c4 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
fix mocks and fixture
0b42138 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
add basic reduce remote write handler benchmark
407e596 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
refactor out common code between write methods
2e57d7e 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
fix: queue manager to include float histograms in new requests
e909eae 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
add sender-side tests and fix failing ones
c7a77af 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
refactor queue manager code to remove some duplication
e70780c 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
fix build
9b4fbd9 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
Improve sender benchmarks and some allocations
c710e19 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
Use github.com/golang/snappy
26673bc 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
cleanup: remove hardcoded fake url for testing
98a0d00 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
Add 1.1 version handling code
d1d80b8 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
Remove config, update proto
3f33175 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
gofmt
7dc2002 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
fix NewWriteClient and change new flags wording
ba3422d 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
fields rewording in handler
55c99d4 
Signed-off-by: Nicolás Pazos <[email protected]>
@npazosmendez
remote write handler to checks version header
ffa3776 
Signed-off-by: Nicolás Pazos <[email protected]>
@bwplotka bwplotka marked this pull request as ready for review July 4, 2024 09:29
@bwplotka bwplotka requested a review from cstyan July 4, 2024 12:15
@bwplotka
Copy link
Copy Markdown
Member Author

bwplotka commented Jul 4, 2024

This is good to merge from my perspective @cstyan @npazosmendez @alexgreenbank 🤗 🎉

bwplotka
bwplotka commented Jul 4, 2024
View reviewed changes
bwplotka
bwplotka commented Jul 4, 2024
View reviewed changes
docs/configuration/configuration.md
@@ -3596,6 +3607,7 @@ write_relabel_configs:
[ send_exemplars: <boolean> | default = false ]
Copy link
Copy Markdown
Member Author

@bwplotka bwplotka Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yolo note: We might want to default true in 3.0

bwplotka
bwplotka commented Jul 4, 2024
View reviewed changes
bwplotka
bwplotka commented Jul 4, 2024
View reviewed changes
bwplotka
bwplotka commented Jul 4, 2024
View reviewed changes
bwplotka
bwplotka commented Jul 4, 2024
View reviewed changes
bwplotka
bwplotka commented Jul 4, 2024
View reviewed changes
bwplotka
bwplotka commented Jul 4, 2024
View reviewed changes
bwplotka
bwplotka commented Jul 4, 2024
View reviewed changes
storage/remote/queue_manager_test.go
samples, newSamples, series := createTimeseriesWithOldSamples(nSamples, nSeries)

c := NewTestWriteClient()
// TODO(alexg): test with new version
Copy link
Copy Markdown
Member Author

@bwplotka bwplotka Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Create issue?

Copy link
Copy Markdown
Member

@cstyan cstyan Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#14415

bwplotka
bwplotka commented Jul 4, 2024
View reviewed changes
cstyan
cstyan approved these changes Jul 4, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@cstyan cstyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we've reviewed this one to death. Merging, we can deal with follow ups.

@cstyan cstyan merged commit 9198952 into main Jul 4, 2024
@cstyan cstyan deleted the remote-write-2.0-main branch July 4, 2024 21:29
bboreham
bboreham reviewed Jul 22, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be clear whether remote-write v1 is deprecated now or in the future.

Personally I think it's dumb to have one option deprecated and one experimental.

docs/configuration/configuration.md
# protobuf message to use when writing to the remote write endpoint.
#
# * The `prometheus.WriteRequest` represents the message introduced in Remote Write 1.0, which
# will be deprecated eventually.
Copy link
Copy Markdown
Member

@bboreham bboreham Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it "deprecated eventually" or "deprecated" ?

documentation/examples/remote_storage/example_write_adapter/README.md
protobuf_message: "io.prometheus.write.v2.Request"
```

or for deprecated Remote Write 1.0 message:
Copy link
Copy Markdown
Member

@bboreham bboreham Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it "deprecated eventually" or "deprecated" ?

config/config.go
}

var (
// RemoteWriteProtoMsgV1 represents the deprecated `prometheus.WriteRequest` protobuf
Copy link
Copy Markdown
Member

@bboreham bboreham Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it "deprecated eventually" or "deprecated" ?

@cstyan cstyan mentioned this pull request Jul 22, 2024
Merged
@cstyan
Copy link
Copy Markdown
Member

cstyan commented Jul 22, 2024

We need to be clear whether remote-write v1 is deprecated now or in the future.

Personally I think it's dumb to have one option deprecated and one experimental.

@bboreham currently it's deprecated eventually. We can change the wording.

I would hope that we can say 2.0 is stable and the default for prom3.0, and change the wording back, but not optimistic on that timeline given how busy those of us working on RW2.0 are. I've opened a PR to clarify the status here: #14499

@rapphil rapphil mentioned this pull request Jul 25, 2024
Open
@tonyli233 tonyli233 mentioned this pull request Sep 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bboreham bboreham bboreham left review comments

@cstyan cstyan cstyan approved these changes

@jesusvazquez jesusvazquez Awaiting requested review from jesusvazquez jesusvazquez is a code owner

@aknuds1 aknuds1 Awaiting requested review from aknuds1 aknuds1 is a code owner

@tomwilkie tomwilkie Awaiting requested review from tomwilkie tomwilkie is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bwplotka @cstyan @bboreham @npazosmendez