Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 142 commits into from
Jul 4, 2024
Merged

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

merged 142 commits into from
Jul 4, 2024

Conversation

bwplotka
Copy link
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 <npazosmendez@gmail.com>

* Prometheus ports automatically assigned

Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>

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

Signed-off-by: Callum Styan <callumstyan@gmail.com>

* Dashboard improvements

* memory stats
* diffs look at counter increases

Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>

* run script: absolute path for config templates

Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>

* grafana dashboard improvements

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

Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>

* dashboard changes

Signed-off-by: Callum Styan <callumstyan@gmail.com>

---------

Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
Signed-off-by: Callum Styan <callumstyan@gmail.com>
Co-authored-by: Callum Styan <callumstyan@gmail.com>
@cstyan
Merge branch 'main' into remote-write-1.1
b36ea85
@cstyan @npazosmendez
replace snappy encoding library
4c4b9aa 
Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@cstyan @npazosmendez
add new proto types
1ac2950 
Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@cstyan @npazosmendez
add decode function for new write request proto
8194000 
Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@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 <callumstyan@gmail.com>
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@cstyan @npazosmendez
Implement code paths for new proto format
ce1e2ad 
Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@cstyan @npazosmendez
update example server to include handler for new format
b7e3665 
Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@cstyan @npazosmendez
Add new test client
91bdd93 
Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
tests and new -> original proto mapping util
005ba7a 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
add new proto support on receiver end
ab7c96a 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
Fix test
7f7cf97 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
no-brainer copypaste but more performance write support
5f5272e 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
remove some comented code
12de4c4 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
fix mocks and fixture
0b42138 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
add basic reduce remote write handler benchmark
407e596 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
refactor out common code between write methods
2e57d7e 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
fix: queue manager to include float histograms in new requests
e909eae 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
add sender-side tests and fix failing ones
c7a77af 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
refactor queue manager code to remove some duplication
e70780c 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
fix build
9b4fbd9 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
Improve sender benchmarks and some allocations
c710e19 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
Use github.com/golang/snappy
26673bc 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
cleanup: remove hardcoded fake url for testing
98a0d00 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
Add 1.1 version handling code
d1d80b8 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
Remove config, update proto
3f33175 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
gofmt
7dc2002 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
fix NewWriteClient and change new flags wording
ba3422d 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
fields rewording in handler
55c99d4 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@npazosmendez
remote write handler to checks version header
ffa3776 
Signed-off-by: Nicolás Pazos <npazosmendez@gmail.com>
@bwplotka bwplotka marked this pull request as ready for review July 4, 2024 09:29
@bwplotka
Fixed writev2.FromMetadataType (was wrongly using prompb).
e044028 
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka bwplotka requested a review from cstyan July 4, 2024 12:15
cstyan added 3 commits July 4, 2024 09:16
@cstyan
fix a few import whitespaces
3302679 
Signed-off-by: Callum Styan <callumstyan@gmail.com>
@cstyan
add a default case with an error to the example RW receiver
8efe7ab 
Signed-off-by: Callum Styan <callumstyan@gmail.com>
@cstyan
more minor import whitespace chagnes
8562b60 
Signed-off-by: Callum Styan <callumstyan@gmail.com>
@bwplotka
Copy link
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
Member Author

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
func TestDropOldTimeSeries(t *testing.T) {
size := 10
nSeries := 6
nSamples := config.DefaultQueueConfig.Capacity * size
samples, newSamples, series := createTimeseriesWithOldSamples(nSamples, nSeries)

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

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
Member

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
bwplotka added 2 commits July 4, 2024 19:23
@bwplotka
Apply suggestions from code review
4adae4d 
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@bwplotka
Update storage/remote/queue_manager_test.go
3668194 
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
cstyan
cstyan approved these changes Jul 4, 2024
View reviewed changes
Copy link
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
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
Member

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
Member

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
Member

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
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