go-grpc-middleware.v2: Action Plan. #275

bwplotka · 2020-02-29T09:19:25Z

As per: #275 ## Change: * Removed grpc_ prefix from all packages like `grpc_middleware`. User can add them on their own. * Moved all specific implementations to separate modules. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

As per: #275 ## Change: * Removed grpc_ prefix from all packages like `grpc_middleware`. User can add them on their own. * Moved all specific implementations to separate modules. Exception is opentracing, as opentracing itself is just amazing interface, so.. perfect (: * Added generic internal interceptor code that both monitoring, logging and tracing will use. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

brancz · 2020-03-02T07:36:21Z

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 · 2020-03-02T08:46:58Z

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 · 2020-03-02T09:24:43Z

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 · 2020-03-02T10:22:22Z

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

As per: #275 The main goals is to have separate modules per each implementation and all reusing same code. This way we can ensure consistency, and we reduce LOC to minimum, including tests. ## Change: * Removed grpc_ prefix from all packages like `grpc_middleware`. User can add them on their own. * Moved all specific implementations to separate modules. Exception is opentracing, as opentracing itself is just amazing interface, so.. perfect (: * Added generic internal interceptor code that both monitoring, logging and tracing will use. * Added scripts for proto generation. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka · 2020-03-02T13:39:09Z

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 · 2020-03-02T17:16:30Z

Not against you mean (:

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

As per: #275 The main goals is to have separate modules per each implementation and all reusing same code. This way we can ensure consistency, and we reduce LOC to minimum, including tests. ## Change: * Removed grpc_ prefix from all packages like `grpc_middleware`. User can add them on their own. * Moved all specific implementations to separate modules. Exception is opentracing, as opentracing itself is just amazing interface, so.. perfect (: * Added generic internal interceptor code that both monitoring, logging and tracing will use. * Added scripts for proto generation. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

As per: #275 The main goals are to have separate modules per each implementation and all reusing same code. This way we can ensure consistency, and we reduce LOC to minimum, including tests. ## Changes: * [x] Removed grpc_ prefix from all packages like `grpc_middleware`. User can add them on their own. * [x] Moved all specific implementations to separate modules. Exception is opentracing, as opentracing itself is just amazing interface, so.. perfect (: * [x] Added generic interceptor code that both monitoring, logging and tracing will use under /interceptors * [x] Reuse generic interceptors in interceptor/{logging,tracing,metrics,tags}. This allows huge simplificaiton of those. * [x] Not context logger. Only ctxtags.Tags responsible for propagating tags in context and proto messages. * [x] Tags are now map[string]string to discourage context value abuse. * [x] Added scripts for proto generation. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

As per: #275 The main goals are to have separate modules per each implementation and all reusing same code. This way we can ensure consistency, and we reduce LOC to minimum, including tests. ## Changes: * [x] Removed grpc_ prefix from all packages like `grpc_middleware`. User can add them on their own. * [x] Moved all specific implementations to separate modules. Exception is opentracing, as opentracing itself is just amazing interface, so.. perfect (: * [x] Added generic interceptor code that both monitoring, logging and tracing will use under /interceptors * [x] Reuse generic interceptors in interceptor/{logging,tracing,metrics,tags}. This allows huge simplificaiton of those. * [x] No context logger. Only ctxtags.Tags responsible for propagating tags in context and proto messages. * [x] Tags are now map[string]string to discourage context value abuse. * [x] Changed tags to allow grpc client inside grpc server tags. * [x] PayloadUnary works as a standalone interceptor as well. * [x] Added scripts for proto generation. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

As per: #275 The main goals are to have separate modules per each implementation and all reusing same code. This way we can ensure consistency, and we reduce LOC to minimum, including tests. ## Changes: * [x] Removed grpc_ prefix from all packages like `grpc_middleware`. User can add them on their own. * [x] Moved all specific implementations to separate modules. Exception is opentracing, as opentracing itself is just amazing interface, so.. perfect (: * [x] Added generic interceptor code that both monitoring, logging and tracing will use under /interceptors * [x] Reuse generic interceptors in interceptor/{logging,tracing,metrics,tags}. This allows huge simplificaiton of those. * [x] No context logger. Only ctxtags.Tags responsible for propagating tags in context and proto messages. * [x] Tags are now map[string]string to discourage context value abuse. * [x] PayloadUnary works as a standalone interceptor as well. * [x] Tags will actually will use map tag instead of noop if not created. * [x] Added scripts for proto generation. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

As per: #275 The main goals are to have separate modules per each implementation and all reusing same code. This way we can ensure consistency, and we reduce LOC to minimum, including tests. ## Changes: * [x] Removed grpc_ prefix from all packages like `grpc_middleware`. User can add them on their own. * [x] Moved all specific implementations to separate modules. Exception is opentracing, as opentracing itself is just amazing interface, so.. perfect (: * [x] Added generic interceptor code that both monitoring, logging and tracing will use under /interceptors * [x] Reuse generic interceptors in interceptor/{logging,tracing,metrics,tags}. This allows huge simplificaiton of those. * [x] No context logger. Only ctxtags.Tags responsible for propagating tags in context and proto messages. * [x] Tags are now map[string]string to discourage context value abuse. * [x] PayloadUnary works as a standalone interceptor as well. * [x] Tags will actually will use map tag instead of noop if not created. * [x] Added scripts for proto generation. * [x] Let tag.Interceptor to extract request data for all types of requests. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka · 2020-03-05T17:19:20Z

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 · 2020-03-09T12:14:51Z

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 · 2020-03-09T14:56:23Z

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 · 2020-03-18T15:08:34Z

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 · 2020-03-18T16:08:17Z

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 · 2020-06-04T11:01:47Z

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 · 2020-06-04T17:42:05Z

Fixing modules in this Pr: #296

XSAM · 2020-06-23T05:51:02Z

I'd like to help this part.

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

bwplotka · 2020-06-25T12:25:09Z

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 · 2020-06-25T12:26:43Z

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

bwplotka · 2020-06-25T13:06:09Z

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 · 2020-07-31T20:54:43Z

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

bwplotka · 2020-08-01T11:26:58Z

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

bwplotka · 2020-10-02T09:51:37Z

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 · 2020-10-06T00:35:47Z

@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 · 2021-01-18T00:40:41Z

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 · 2021-03-12T03:43:17Z

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

yashrsharma44 · 2021-03-12T04:12:31Z

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 · 2021-03-12T11:13:00Z

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

bwplotka self-assigned this Feb 29, 2020

bwplotka mentioned this issue Feb 29, 2020

Initial change for v2. #276

Merged

10 tasks

bwplotka added enhancement help wanted labels Mar 5, 2020

bwplotka mentioned this issue Jun 9, 2020

feat: Move api & client to a separate module prometheus-operator/prometheus-operator#3269

Closed

yashrsharma44 mentioned this issue Jun 22, 2020

Upgrade grpc middleware to v2 thanos-io/thanos#2788

Closed

2 tasks

XSAM mentioned this issue Jun 30, 2020

Refactor tracing implements #312

Closed

bwplotka added the v2 label Oct 2, 2020

bwplotka closed this as completed Oct 2, 2020

iamrajiv mentioned this issue Dec 10, 2020

Added Dependabot #373

Closed

iamrajiv mentioned this issue Jan 1, 2021

Added Dependabot #376

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go-grpc-middleware.v2: Action Plan. #275

go-grpc-middleware.v2: Action Plan. #275

bwplotka commented Feb 29, 2020 •

edited by yashrsharma44

brancz commented Mar 2, 2020

bwplotka commented Mar 2, 2020

brancz commented Mar 2, 2020 •

edited

bwplotka commented Mar 2, 2020 via email

bwplotka commented Mar 2, 2020

brancz commented Mar 2, 2020

bwplotka commented Mar 5, 2020

pgbytes commented Mar 9, 2020

bwplotka commented Mar 9, 2020 via email

mattbailey commented Mar 18, 2020

bwplotka commented Mar 18, 2020 via email

irridia commented Jun 4, 2020

bwplotka commented Jun 4, 2020

XSAM commented Jun 23, 2020

bwplotka commented Jun 25, 2020

johanbrandhorst commented Jun 25, 2020

bwplotka commented Jun 25, 2020 via email

irridia commented Jul 31, 2020

bwplotka commented Aug 1, 2020

bwplotka commented Oct 2, 2020 •

edited

ash2k commented Oct 6, 2020 •

edited

drewwells commented Jan 18, 2021

codefromthecrypt commented Mar 12, 2021

yashrsharma44 commented Mar 12, 2021

ash2k commented Mar 12, 2021

go-grpc-middleware.v2: Action Plan. #275

go-grpc-middleware.v2: Action Plan. #275

Comments

bwplotka commented Feb 29, 2020 • edited by yashrsharma44

Work Done

TODO (help wanted!) for 2.0:

Blockers:

Can be done in 2.x

brancz commented Mar 2, 2020

bwplotka commented Mar 2, 2020

brancz commented Mar 2, 2020 • edited

bwplotka commented Mar 2, 2020 via email

bwplotka commented Mar 2, 2020

brancz commented Mar 2, 2020

bwplotka commented Mar 5, 2020

pgbytes commented Mar 9, 2020

bwplotka commented Mar 9, 2020 via email

mattbailey commented Mar 18, 2020

bwplotka commented Mar 18, 2020 via email

irridia commented Jun 4, 2020

bwplotka commented Jun 4, 2020

XSAM commented Jun 23, 2020

bwplotka commented Jun 25, 2020

johanbrandhorst commented Jun 25, 2020

bwplotka commented Jun 25, 2020 via email

irridia commented Jul 31, 2020

bwplotka commented Aug 1, 2020

bwplotka commented Oct 2, 2020 • edited

ash2k commented Oct 6, 2020 • edited

drewwells commented Jan 18, 2021

codefromthecrypt commented Mar 12, 2021

yashrsharma44 commented Mar 12, 2021

ash2k commented Mar 12, 2021

bwplotka commented Feb 29, 2020 •

edited by yashrsharma44

brancz commented Mar 2, 2020 •

edited

bwplotka commented Oct 2, 2020 •

edited

ash2k commented Oct 6, 2020 •

edited