go-grpc-middleware.v2: Action Plan.

[bwplotka]

Hi 👋

We have tons of stuff to improve in this library and there wasn't really time for this. Let's do it finally! Let's maintain this issue as a single place to track our work. 💪 cc @domgreen @johanbrandhorst @mwitkow

The plan is to start from scratch in the v2 branch. We will work in the background until it's done.

Work Done

• Started v2 branch, implemented all while leaving same API. Assignee: @bwplotka

• Never again use those weird module_names 🙃 Done Assignee: @bwplotka

• Multi-module architecture, allowing us to avoid dependency hell, but also retaining mono repo. Module structure: Done Assignee: @bwplotka
  The key part is to make sure the core is never importing the provider's code. There has to be indirection. This way the lib has minimum dependencies possible, with core literally having core only.

  If maintaining multi modules will be problematic, we can look into modularise cc @Helcaraxan

TODO (help wanted!) for 2.0:

Blockers:

• Do one more review of all interfaces we expose to check if we can simplify something. Assignee @johanbrandhorst
• Upgrade gRPC and use https://godoc.org/google.golang.org/grpc#ChainUnaryInterceptor Assignee: @bwplotka
• Upgrade golang/protobuf to v2. Same with gogo? Assignee: @bwplotka
• Move to GitHub actions Assignee: @bwplotka
• MIgration docs!
• Start using gorelease (https://godoc.org/golang.org/x/exp/cmd/gorelease)
• Consider adding whitelist behaviour to auth interceptor: How to enable whitelist auth?
• Add tracing interface and move opentracing to provides. Add opentelemetry providers.
• Use the v2 in Thanos for first hand expierience in upgrading. Assignee: @bwplotka

Can be done in 2.x

• Move https://github.com/grpc-ecosystem/go-grpc-prometheus as another provider. Deprecate repo.
• Add failint to avoid not needed imports (e.g v1 packages)
• Move to more maintained validators
• Review if we can simplify retry internally
• Stale bot
• Dependabot

Any other ideas?

[brancz]

I have no strong opinion about essentially having a monorepo, but I do wonder what the benefit is. I think they were separate because they had nothing in common not even shared unit test framework or anything. So I wonder if it’s worth it considering that given that things become harder with modules...

More generally though I do think these things need a pretty major overhaul and would love to see that happen! :)

[bwplotka]

Thanks for the feedback!

I have no strong opinion about essentially having a monorepo, but I do wonder what the benefit is. I think they were separate because they had nothing in common not even shared unit test framework or anything.

Now (as you will see in #276) all middlewares with reuse the majority of its code. Same for test frameworks (: It will be then easier for all maintainers (including Dom, Johan) to understand Prometheus one.

So I wonder if it’s worth it considering that given that things become harder with modules...

Yes, but in the same time we have to solve it for logrus, zap, go-kit and potentially other implementations (: So far it's quite easy to work with multiple modules, but let's see the pain points (:

[brancz]

Yeah loggers would all be in the same repo, I'm more thinking things that are more distinct like logging and metrics middleware. But as I said, I'm not against trying this at all.

[bwplotka]

Not against you mean (:

…

On Mon, 2 Mar 2020 at 09:24, Frederic Branczyk ***@***.***> wrote: Yeah loggers would all be in the same repo, I'm more thinking things that are more distinct like logging and metrics middleware. But as I said, I'm against trying this at all. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#275?email_source=notifications&email_token=ABVA3O7SELUGU3RKVE4CNCDRFN3NZA5CNFSM4K6STMT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENORY3Y#issuecomment-593304687>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3O344RE4XJQAMC7SRLLRFN3NZANCNFSM4K6STMTQ> .

[bwplotka]

Essentially this is only what all implementations (including Prometheus) will need to implement and test (!):

type ClientReportable interface {
	ClientReporter(ctx context.Context, typ GRPCType, serviceName string, methodName string) (Reporter, context.Context)
}

type ServerReportable interface {
	ServerReporter(ctx context.Context, typ GRPCType, serviceName string, methodName string) (Reporter, context.Context)
}

type Reporter interface {
	PostCall(error, time.Duration)

	PostMsgSend(error, time.Duration)
	PostMsgReceive(error, time.Duration)
}

[brancz]

Not against you mean (:

whops, what a great typo.. :D fixed my comment

[bwplotka]

First to points are done thanks to this: #276

Added few more todos as well. Please propose PRs to v2 if you want to help 🤗

[pgbytes]

TODO suggestion:

grpc_logrus.Decider currently takes fullMethodName string and err error as parameters, that restricts us in filtering by only two arguments. Would be nice to get the context in this method, so that we can filter by more things, such as the IP address of requester.

[bwplotka]

Definitely a valid request. 👍

…

On Mon, 9 Mar 2020 at 12:14, Panshul Gupta ***@***.***> wrote: TODO suggestion: grpc_logrus.Decider currently takes fullMethodName string and err error as parameters, that restricts us in filtering by only two arguments. Would be nice to get the context in this method, so that we can filter by more things, such as the IP address of requester. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#275?email_source=notifications&email_token=ABVA3O7D4QBKDSVZI6QBJTTRGTMTZA5CNFSM4K6STMT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOG3XBQ#issuecomment-596491142>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3O5ORSFKEALZY4BXQYDRGTMTZANCNFSM4K6STMTQ> .

[mattbailey]

I'd like to see an interceptor that uses grpclog

It might be effective to, instead of writing an interceptor for each log library, write one for grpclog and then write interface shims for log libraries to satisfy the grpclog.LoggerV2 interface.

[bwplotka]

We already did that (:

…

On Wed, 18 Mar 2020 at 15:08, Matt Bailey ***@***.***> wrote: I'd like to see an interceptor that uses grpclog It might be effective to, instead of writing an interceptor for each log library, write one for grpclog and then write interface shims for log libraries to satisfy the grpclog.LoggerV2 interface. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#275 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3OZDSE5A5DYFNIFRKH3RIDPYHANCNFSM4K6STMTQ> .

[irridia]

FWIW, I've had luck with multiple modules in a repo, but non-nested and without the v2+ go module "feature". I did suspect that the /v2/ needs to be at the end of the nested modules, but I wasn't 100% sure it wasn't part of Go's Very Well Documented Module Magick. :-|

I tried playing around with this in my fork, but it got very complicated. I can also poke at it later today after I get some sleep.

[bwplotka]

Fixing modules in this Pr: #296

[XSAM]

I'd like to help this part.

Add tracing interface and move opentracing to provides. Add opentelemetry providers.

[bwplotka]

Please help @XSAM (:

Also @johanbrandhorst we were having an interesting discussion with @yashrsharma44

For example, all our intercepters are using a solid reporter interface. Such a reporter is not really tied to gRPC which gives us a quick way to implement tripperware and middleware for pure HTTP code as well that ensure the same metric, logging, tracing and tags behavior among HTTP and gRPC. This, however, would mean that:

• interceptors/logging would need to be more agnostic to gRPC (e.g Code to Level might be int code, not codes.Code etc)
• we might want to even host HTTP version of interceptors. This sounds quite crazy and is far from scope of this library, but some sibling projects would make sense - as long as we allow reusing our "reporters" for both gRPC interceptors and HTTP middleware.

What do you think? (:

[johanbrandhorst]

I think there's a real danger of going to deep on abstractions. Lets keep it simple if we can.

[bwplotka]

Yes, especially now there is not much code. This is something to think about. (: For now we plan to logging into Thanos directly and adjust for HTTP.

…

On Thu, 25 Jun 2020 at 14:26, Johan Brandhorst ***@***.***> wrote: I think there's a real danger of going to deep on abstractions. Lets keep it simple if we can. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#275 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3O4BD3OJ5NTXEXZYA5TRYM7BDANCNFSM4K6STMTQ> .

[irridia]

Just FYI, I've been using the logging interceptors (zerolog specifically) in prod with zero issues.

[bwplotka]

Amazing, looking for more time to finish things up (: protobuf and grpc nearly updated to latest

[bwplotka]

I pulled all the tasks to separate issues, so the idea is for @yashrsharma44 to create GH project for first v2 release and track outstanding items there. (:

[ash2k]

@bwplotka I'm glad to see v2 happening!

For metrics, have you considered using gRPC's stats.Handler (via grpc.StatsHandler() for server and grpc.WithStatsHandler() for dials) instead of interceptors?

https://github.com/piotrkowalczuk/promgrpc uses stats.Handler API.

p.s. I'm glad that v2 does not have any global state and does not register with the global prometheus registry!

[drewwells]

Amazing, looking for more time to finish things up (: protobuf and grpc nearly updated to latest

Where is outstanding working being tracked? I was looking at the list in this issue and noticed some where already done like github-actions.

[codefromthecrypt]

Is it time for another release candidate? last was 9 months ago

[yashrsharma44]

For metrics, have you considered using gRPC's stats.Handler (via grpc.StatsHandler() for server and grpc.WithStatsHandler() for dials) instead of interceptors?

@ash2k Is there any difference on using StatsHandler instead of the interceptors for gRPC Middlewares? Just curious to know more about them.

[ash2k]

@yashrsharma44 I'm not the best person to ask as I'm not a gRPC expert, but off the top of my head:

• I suppose if gRPC team introduced a special way to collect stats, the approach must have some clear benefits.
• Interceptors are invoked in some order. If an interceptor that sits before the stats interceptor aborts the request, then the stats interceptor does not get a chance to measure the call.
• It feels cleaner to use a separate API for stats if it's there than mixing it with interceptors.
• Less chance of incorrect interceptor ordering.