Update prometheus/exporter-toolkit to v0.8.2 and gRPC to 1.47 by rabenhorst · Pull Request #262 · weaveworks/common

rabenhorst · 2022-11-14T11:16:51Z

Updated prometheus/exporter-toolkit to v0.8.2 to make it work with Prometheus v0.40.1.

bboreham

Can we avoid bumping gRPC?
Also, did you go mod tidy ? Seems like a lot of lines added in go.sum compared to actual change.

bboreham · 2022-11-23T14:07:58Z

go.mod

-	google.golang.org/grpc v1.31.0
+	golang.org/x/net v0.0.0-20220909164309-bea034e7d591
+	golang.org/x/tools v0.1.5
+	google.golang.org/grpc v1.47.0


Can it be done without this update? This is an enormous jump.

If not, someone needs to do forwards/backwards compatibility testing to confirm that old and new clients work against old and new servers.

Actually mimir has been running with 1.45 grpc since 14th June grafana/mimir#2098

We've concluded that Mimir's usage of grpc is not proof and I'm going to reuse the server test code (httpgrpc/server_test, with a few additions) to test compatibility of versions:
client using 1.31 grpc -> server 1.47
client 1.47 -> server 1.31

What I did to test gprc message handling compatibility:

Extracted the client and server sides of the httpgrpc/server/server.go and httpgrpc/server/server_test.go` files into https://github.com/krajorama/weaveworks-common-testclient and https://github.com/krajorama/weaveworks-common-testserver .

Added a test to return the OrgId extracted on the server side in the body . And another to test sending a method different then GET.

To run a test I execute the server and copied the URL it listened on, then run the client tests with:
weaveworks-common-testclient$ SERVER_URL=direct://127.0.0.1:35635 go test -count 1 -v ./...

The baseline was using the latest weaveworks/common at v0.0.0-20221125125352-88dcecd133b1, where:

require ( ... github.com/weaveworks/common v0.0.0-20221125125352-88dcecd133b1 google.golang.org/grpc v1.31.0 )

Then doing a go.mod replace to get:

replace github.com/weaveworks/common v0.0.0-20221125125352-88dcecd133b1 => github.com/rabenhorst/common v0.0.0-20221130173339-6d74945c9349 require ( ... github.com/weaveworks/common v0.0.0-20221125125352-88dcecd133b1 google.golang.org/grpc v1.47.0 )

I've run the test with all combinations: client version x server version. All tests passed.

bboreham · 2022-11-23T14:10:10Z

Thanks for the PR, by the way.

bboreham · 2022-11-23T14:43:40Z

Looks like the chain of dependencies that updates gRPC is:

github.com/prometheus/exporter-toolkit@v0.8.1
-> golang.org/x/oauth2@v0.0.0-20220909003341-f21342109be1 here
-> cloud.google.com/go/compute@v1.7.0 here
-> google.golang.org/grpc@v1.47.0 here

I don't see a way to break that, except larger-scale copy-paste.

krajorama · 2022-11-23T18:29:28Z

This is actually needed for native historgrams work as well in mimir-prometheus.

krajorama

Please run make protos and add the result to the PR, thanks

krajorama · 2022-11-24T09:10:21Z

Please run make protos and add the result to the PR, thanks

Actually this lead to errors for Mimir, so let's forget it for now. I'm trying to figure out what happened.

krajorama · 2022-11-24T14:16:54Z

We should merge #264 first. Fixes the make protos issue above.

rabenhorst · 2022-11-29T15:24:09Z

We should merge #264 first. Fixes the make protos issue above.

I rebased on master, ran make protos on both macOS arm and linux x86 machine and always get an error:

gogoproto/gogo.proto: File not found.
httpgrpc/httpgrpc.proto:5:1: Import “gogoproto/gogo.proto” was not found or had errors.

Any idea how to fix this?

cristiangreco · 2022-11-29T17:35:04Z

Drive-by: prometheus/exporter-toolkit@v0.8.2 has been released today, which fixes https://pkg.go.dev/vuln/GO-2022-1130

krajorama · 2022-11-30T07:47:15Z

We should merge #264 first. Fixes the make protos issue above.

I rebased on master, ran make protos on both macOS arm and linux x86 machine and always get an error:
gogoproto/gogo.proto: File not found.
httpgrpc/httpgrpc.proto:5:1: Import “gogoproto/gogo.proto” was not found or had errors.
Any idea how to fix this?

Put the path to gogoproto/gogo.proto in the -I include path of protoc compiler. (Since it is already a dependency of thanos-io).

E.g. in mimir:
https://github.com/grafana/mimir/blob/0f2929359b8b02b8427f1a901839b1805f43d58b/Makefile#L243

But since you're not using vendoring, you might have to do like in prometheus/alertmanager:
https://github.com/prometheus/alertmanager/blob/6ef6e6868dbeb7984d2d577dd4bf75c65bf1904f/Makefile.common#L241
https://github.com/prometheus/alertmanager/blob/main/scripts/genproto.sh

And possibly add a tools.go to make go get actually download it :(
https://github.com/weaveworks/common/blob/master/httpgrpc/tools.go

Background: we found that make protos here in weaveworks/common did not reproduce the generated files since it was missing the gogo extension definitions. Now this is added so CI can check protobuf definitions and generated files automatically.

rabenhorst · 2022-11-30T17:33:02Z

Put the path to gogoproto/gogo.proto in the -I include path of protoc compiler. (Since it is already a dependency of thanos-io).

Thx. It worked and there is no change in generated files. Is this expected?

Also I updated exporter-toolkit to v0.8.2.

Signed-off-by: Sebastian Rabenhorst <sebastian.rabenhorst@shopify.com>

krajorama · 2022-12-01T08:27:38Z

Put the path to gogoproto/gogo.proto in the -I include path of protoc compiler. (Since it is already a dependency of thanos-io).

Thx. It worked and there is no change in generated files. Is this expected?

Also I updated exporter-toolkit to v0.8.2.

Yes, that's expected, we just wanted to get to a reproducable build where make protos does not result in diffs in git.

bboreham

As noted in individual comments, this is a big jump in gRPC version, and the stats say something has changed on the wire, but I'm very grateful to @krajorama for testing backwards and forwards compatibility.

bboreham · 2022-12-01T10:28:06Z

middleware/grpc_stats_test.go

 			received_payload_bytes_bucket{method="gRPC",route="/middleware.EchoServer/Process",le="2.62144e+08"} 5
 			received_payload_bytes_bucket{method="gRPC",route="/middleware.EchoServer/Process",le="+Inf"} 5
-			received_payload_bytes_sum{method="gRPC",route="/middleware.EchoServer/Process"} 1.5728664e+07
+			received_payload_bytes_sum{method="gRPC",route="/middleware.EchoServer/Process"} 1.5728689e+07


Noting this difference suggests to me the code is seeing 5 bytes more on the wire.

This turned out to be an artifact from this fix in grpc: grpc/grpc-go#3886 , the old code was reporting wrong wirelength to the statistics callback actually.

…mmon` (#8125) Not needed after weaveworks/common#254 and weaveworks/common#262 More rationale on the issue. Fixes #8085 Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

rabenhorst mentioned this pull request Nov 16, 2022

Update Prometheus to v0.40.1 thanos-io/thanos#5896

Merged

2 tasks

bboreham reviewed Nov 23, 2022

View reviewed changes

bboreham mentioned this pull request Nov 23, 2022

[chore] Update Prometheus to v0.40.1 open-telemetry/opentelemetry-collector-contrib#16342

Closed

krajorama mentioned this pull request Nov 23, 2022

Update exporter-toolkit to v0.8.1 #263

Closed

krajorama reviewed Nov 23, 2022

View reviewed changes

krajorama mentioned this pull request Nov 28, 2022

Prometheus Native Histogram support phase1 grafana/mimir#3478

Closed

rabenhorst added 2 commits November 29, 2022 14:36

Updated prometheus/exporter-toolkit to v0.8.1

d8fdf62

Fixed tests

20964de

rabenhorst force-pushed the prometheus-exporter-toolkit-v0.8.1 branch from 9eb1f14 to 20964de Compare November 29, 2022 14:04

This was referenced Nov 29, 2022

Add support for Prometheus native histograms grafana/agent#2492

Closed

Update Prometheus dependency to v2.40 grafana/loki#7811

Closed

rabenhorst changed the title ~~Updated prometheus/exporter-toolkit to v0.8.1~~ Updated prometheus/exporter-toolkit to v0.8.2 Nov 30, 2022

Updated exporter toolkit to v0.8.2

6d74945

Signed-off-by: Sebastian Rabenhorst <sebastian.rabenhorst@shopify.com>

bboreham approved these changes Dec 1, 2022

View reviewed changes

bboreham merged commit 7c2720a into weaveworks:master Dec 1, 2022

bboreham changed the title ~~Updated prometheus/exporter-toolkit to v0.8.2~~ Update prometheus/exporter-toolkit to v0.8.2 and gRPC to 1.47 Dec 1, 2022

krajorama mentioned this pull request Dec 2, 2022

Update Prometheus dependency to latest release (v2.40.4) grafana/loki#7826

Merged

This was referenced Jan 10, 2023

unnecessary replace in go.mod for weaveworks/common grafana/loki#8085

Closed

unnecessary replace in go.mod for weaveworks/common cortexproject/cortex#5087

Closed

kavirajk mentioned this pull request Jan 12, 2023

chore(go.mod): Remove unnecessary grpc contraints from weaveworks/common grafana/loki#8125

Merged

1 task

Conversation

rabenhorst commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

bboreham Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

krajorama Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

krajorama Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

krajorama Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

bboreham commented Nov 23, 2022

Uh oh!

bboreham commented Nov 23, 2022

Uh oh!

krajorama commented Nov 23, 2022

Uh oh!

krajorama left a comment

Choose a reason for hiding this comment

Uh oh!

krajorama commented Nov 24, 2022

Uh oh!

krajorama commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rabenhorst commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristiangreco commented Nov 29, 2022

Uh oh!

krajorama commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rabenhorst commented Nov 30, 2022

Uh oh!

krajorama commented Dec 1, 2022

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

bboreham Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

krajorama Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rabenhorst commented Nov 14, 2022 •

edited

Loading

krajorama commented Nov 24, 2022 •

edited

Loading

rabenhorst commented Nov 29, 2022 •

edited

Loading

krajorama commented Nov 30, 2022 •

edited

Loading