thanos icon indicating copy to clipboard operation
thanos copied to clipboard

Update go-grpc-middleware to v2.0.0

Open coleenquadros opened this issue 2 years ago • 7 comments

  • Update go_grpc_middleware to v2.0.0.
  • Remove Tags Interceptor from Thanos. Tags interceptor is removed from v2.0.0 go-grpc-middleware.
  • Tracing interceptor directory from go_grpc_middleware(v2.0.0 rc 2,3,4) moved to Thanos(v2.0.0 makes use of otel). Tracing interceptor was removed in version v2.0.0. The follow up in Thanos would be to migrate to otel.
  • Logging middleware migrated to use v2.0.0 logging interceptors. Trace ID is now logged as a part of GRPC request logging(output below). (PR for adding trace ID to HTTP logging middleware - https://github.com/thanos-io/thanos/pull/6611)
ts=2023-10-20T11:34:12.98939Z caller=grpc.go:107 level=debug name=store service=gRPC/server component=store msg="started call" traceID=5ee8eb9e1b05437670f2d311252aa755 requestID=01HD6DTJQXHWXEHBS7YE7QVJ22 protocol=grpc grpc.component=server grpc.service=thanos.info.Info grpc.method=Info grpc.method_type=unary peer.address=127.0.0.1:52802 grpc.start_time=2023-10-20T13:34:12+02:00 grpc.request.deadline=2023-10-20T13:34:17+02:00 grpc.time_ms=0.145
ts=2023-10-20T11:34:12.989388Z caller=grpc.go:107 level=debug name=store service=gRPC/server component=store msg="started call" traceID=d25ad23a02c034c84fc856a4d6a59fdb requestID=01HD6DTJQX368WC3KT5R45H4WY protocol=grpc grpc.component=server grpc.service=thanos.info.Info grpc.method=Info grpc.method_type=unary peer.address=127.0.0.1:52801 grpc.start_time=2023-10-20T13:34:12+02:00 grpc.request.deadline=2023-10-20T13:34:17+02:00 grpc.time_ms=0.137
ts=2023-10-20T11:34:12.989441Z caller=grpc.go:107 level=debug name=store service=gRPC/server component=store msg="finished call" traceID=5ee8eb9e1b05437670f2d311252aa755 requestID=01HD6DTJQXHWXEHBS7YE7QVJ22 protocol=grpc grpc.component=server grpc.service=thanos.info.Info grpc.method=Info grpc.method_type=unary peer.address=127.0.0.1:52802 grpc.start_time=2023-10-20T13:34:12+02:00 grpc.request.deadline=2023-10-20T13:34:17+02:00 grpc.code=OK grpc.time_ms=0.2
ts=2023-10-20T11:34:12.989445Z caller=grpc.go:107 level=debug name=store service=gRPC/server component=store msg="finished call" traceID=d25ad23a02c034c84fc856a4d6a59fdb requestID=01HD6DTJQX368WC3KT5R45H4WY protocol=grpc grpc.component=server grpc.service=thanos.info.Info grpc.method=Info grpc.method_type=unary peer.address=127.0.0.1:52801 grpc.start_time=2023-10-20T13:34:12+02:00 grpc.request.deadline=2023-10-20T13:34:17+02:00 grpc.code=OK grpc.time_ms=0.199

  • [x] I added CHANGELOG entry for this change.
  • [ ] Change is not relevant to the end user.

Changes

Verification

coleenquadros avatar Aug 24 '23 12:08 coleenquadros

@yeya24, @matej-g any chance we could get a review to try to unblock go-grpc-middleware upgrade?

moadz avatar Sep 27 '23 09:09 moadz

It would be great if we can move this forward. I am kind of blocked by this right now.

yeya24 avatar Oct 17 '23 06:10 yeya24

Looks like unit tests are still failing.

fpetkovski avatar Oct 18 '23 13:10 fpetkovski

The branch is failing due to this test case in the go-grpc-middleware v2.0.0.rc.2 which was copied over - https://github.com/grpc-ecosystem/go-grpc-middleware/blob/880c1749170035e5a54794546dbfe0d45d10a11d/interceptors/tracing/interceptors_test.go#L78


This is failing in the v2.0.0.-rc.2 tag too

`
=== RUN   TestTaggingSuite
    interceptor_suite.go:127: 
        	Error Trace:	/home/samukher/web/thanos/pkg/tracing/tracing_middleware/grpctesting/interceptor_suite.go:127
        	            				/home/samukher/web/thanos/pkg/tracing/tracing_middleware/grpctesting/interceptor_suite.go:93
        	            				/usr/local/go/src/runtime/asm_amd64.s:1650
        	Error:      	Received unexpected error:
        	            	context deadline exceeded
        	Test:       	TestTaggingSuite
        	Messages:   	must not error on client Dial
panic: test timed out after 15m0s
running tests:
	TestTaggingSuite (15m0s)
`

coleenquadros avatar Oct 20 '23 09:10 coleenquadros

@coleenquadros looks like there is a final and very small linting issue left to be fixed -- just the ordering of imports in a file.

douglascamata avatar Nov 09 '23 14:11 douglascamata

Could we get another round of reviews, please? 🥺

douglascamata avatar Nov 20 '23 10:11 douglascamata

@coleenquadros would you mind rebasing and we can merge this PR.

fpetkovski avatar May 03 '24 07:05 fpetkovski

@fpetkovski @pedro-stanaka I have rebased the changes. Sorry for the delay in response.

coleenquadros avatar May 27 '24 11:05 coleenquadros