-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
Implement capnproto replication #7659
Conversation
253f417
to
7042bea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this include a whole new RPC framework? I'm very interested in this. Glad to finally see #7071 tackled. How do you think this compared to dRPC? And is it worth eschewing gRPC altogether?
Really interested in this too! Our profiles show similar for unmarshaling |
Yes this comes with its own RPC framework which can be a double edge sword. It's one more thing to learn and troubleshoot if it goes wrong. I haven't looked into dRPC yet in practice, but maybe there is a way to abstract the RPC framework and not expose internals to end users, this way we can swap it out in future releases in a transparent manner. In this PR we expose the listen port as a parameter since we need separate connections for Cap'n Proto. |
DRPC allows serving both gRPC and DRPC on the same socket to allow for incrementally upgrading/migrating a fleet. We might consider doing something similar whether we go with capnproto or DRPC |
Mhm, we probably need to experiment more with the transparent approach before deciding what to do. For example, we could maybe deprecate My only suggestion would be:
Instead of having two addresses here, let's have |
Perhaps we can use something like https://github.com/soheilhy/cmux to multiplex multiple protocols on the same port. This way we don't need to pollute arguments with flags for each protocol. Let me see if this is easily doable, and also add some tests for the new RPC. |
d22809b
to
5cfae4c
Compare
3df360a
to
2fdeba7
Compare
b46ec5d
to
79a7aa2
Compare
2bea7c9
to
b340ea0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments and questions.
Apart from the stuff I pointed out, if I am getting this right, we won't have metrics for the ingestion process. Should we add some metrics for the most critical paths?
And to keep my MO, I leave that question: Can we add this to the changelog? 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is the generation target for this in makefile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have this yet because it's not easy to automate installation of the capnproto generator. Maybe we can add instructions for installations in the README, and the Makefile can assume the binary exists on the local machine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it just yet another bingo tool? go install capnproto.org/go/capnp/v3/capnpc-go@latest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to install the tools from https://capnproto.org/install.html. It also requires the capnp-go repository to be cloned locally for the schemas.
Here are the docs: https://github.com/capnproto/go-capnp/blob/main/docs/Installation.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a make target which should do most of the heavy lifting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can test somehow that if prometheus ever evolved their schemas, we still would be compatible here. maybe a simple e2e test with PRW?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have this in my branch, about to open up a PR for this
|
||
var lset labels.Labels | ||
// Check if the TSDB has cached reference for those labels. | ||
ref, lset = getRef.GetRef(series.Labels, series.Labels.Hash()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other writer has a bunch of validations of over labels, like if we have duplicate labels or labels are missing __name__
, do we need that here?
I dont see those validations on the marshal part of the captnproto, that is why I ask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were sorting labels in the unmarshal method which I though removed the need for validation.
But since there's more than just to validate, I removed the sort and we do the validation in the writer now.
Long term we should do this in the router to keep CPU usage in the ingestor lower.
// Nodes returns a sorted slice of nodes that are in this hashring. Addresses could be duplicated | ||
// if, for example, the same address is used for multiple tenants in the multi-hashring. | ||
Nodes() []string | ||
Nodes() []Endpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a breaking change for people using this as library, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is technically a breaking change, but I don't recall having any guarantees on using Thanos as a library.
Adding e2e test: fpetkovski#7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lg
b.Run("unmarshal", func(b *testing.B) { | ||
for i := 0; i < b.N; i++ { | ||
msg, err := capnp.Unmarshal(bs) | ||
require.NoError(b, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gives me:
/home/giedriusstatkevicius/dev/thanos/pkg/receive/writecapnp/marshal_bench_test.go:101:
Error Trace: /home/giedriusstatkevicius/dev/thanos/pkg/receive/writecapnp/marshal_bench_test.go:101
/usr/local/go/src/testing/benchmark.go:193
/usr/local/go/src/testing/benchmark.go:215
/usr/local/go/src/runtime/asm_amd64.s:1700
Error: Received unexpected error:
unmarshal: short header section
Does this benchmark work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we are unmarshaling proto bytes into a capnproto request. I've fixed the benchmark, all of them should be passing now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make docs
failing 😬
Our profiles from production show that a lot of CPU and memory in receivers is used for unmarshaling protobuf messages. Although it is not possible to change the remote-write format, we have the freedom to change the protocol used for replicating timeseries data. This commit introduces a new feature in receivers where replication can be done using Cap'n Proto instead of gRPC + Protobuf. The advantage of the former protocol is that deserialization is far cheaper and fields can be accessed directly from the received message (byte slice) without allocating intermediate objects. There is an additional cost for serialization because we have to convert from Protobuf to the Cap'n proto format, but in our setup this still results in a net reduction in resource usage. Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Co-authored-by: Pedro Tanaka <[email protected]> Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
4b34a07
to
d337e0a
Compare
Rebased on main and regenerated docs. We should be good to go! We can keep iterating over time, I'd like to see how we can leverage interning in the future. |
Our profiles from production show that a lot of CPU and memory in receivers is used for unmarshaling protobuf messages. Although it is not possible to change the remote-write format, we have the freedom to change the protocol used for replicating timeseries data.
This commit introduces a new feature in receivers where replication can be done using Cap'n Proto instead of gRPC + Protobuf. The advantage of the former protocol is that deserialization is far cheaper and fields can be accessed directly from the received message (byte slice) without allocating intermediate objects. There is an additional cost for serialization because we have to convert from Protobuf to the Cap'n proto format, but in our setup this still results in a net reduction in resource usage.
We have a split router-receiver setup and this is our resource usage in staging after enabling the new replication method:
Router usage did go up as well, but we still got an overall net reduction:
We can also experiment with other formats by having a more generic flag:
receive.replication-format=...
Changes
Verification