Hi :wave:

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

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

• [x] Never again use those weird module_names :upside_down_face: Done Assignee: @bwplotka

• [X] 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

• [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?

This PR achieves to add in a Request/Response logger in grpc_middlewares v2 branch.

Fixes #310

Current TODO -

• [x] ~~Initial setup for the RequestReporter~~ Configure the request logger in the old logger itself
• [x] Write tests for the new reporter
• [x] Write examples for the new reporter
• [x] Decide what to log after we can access the request/response object cc @bwplotka ref: https://github.com/grpc-ecosystem/go-grpc-middleware/pull/311/files#diff-0c98eb4ff4e54e07ef0a999e5d5bd678R137
• [x] Naming of the reporter - Is the current naming correct, or do we want something else :stuck_out_tongue:

If any suggestions are there, feel free to add in more :sweat_smile:

Signed-off-by: Yash Sharma [email protected]

Greetings 👋

I come from the Thanos project where we mostly use the go-kit logging library and we wanted some sweet gRPC logging for our users.

I'm not a Go expert, so I mostly copy/pasted the tests from the Zap implementation and wrote the go-kit logging system.

The tests suite is green and it works pretty swell with Thanos:

level=debug ts=2019-08-09T12:24:01.299269Z caller=storeset.go:223 component=storeset msg="updating healthy stores" externalLabelOccurrencesInStores="map[string]int{\"\":1}"
level=info ts=2019-08-09T12:24:01.299296Z caller=storeset.go:266 component=storeset msg="adding new store to query storeset" address=127.0.0.1:10904
level=info ts=2019-08-09T12:24:06.300377Z caller=payload_interceptors.go:134 system=grpc span.kind=client grpc.service=thanos.Store grpc.method=Info grpc.request.content={}
level=info ts=2019-08-09T12:24:06.302309Z caller=payload_interceptors.go:134 system=grpc span.kind=client grpc.service=thanos.Store grpc.method=Info grpc.response.content="{\"maxTime\":\"9223372036854775807\",\"storeType\":\"SIDECAR\"}"
level=debug ts=2019-08-09T12:24:06.302375Z caller=client_interceptors.go:49 system=grpc span.kind=client grpc.service=thanos.Store grpc.method=Info msg="finished client unary call" error=null grpc.code=OK grpc.time_ms=2.145```

Let me know if you have any feedback 😄 

We at Hootsuite were noticing noisy logs coming from grpc and we noticed it's because grpc_zap implements logger v1 interface. We need the logger v2 implemented instead, so I have opened this PR.

This is a revived and comments addressed version of #144 which was opened by @jamisonhyatt. Feel free to just merge this in after approval if it looks good!

Kind of biggy, but should be much cleaner.

The only surprise is that tracing is using logging.Fields to both fill span metadata (trace ID, sampled or not, span ID) as well. I think this is useful AND it was like that before 🤗

At least it's a clean separation of responsibilities who is using what vs generic tags.

Fixes https://github.com/grpc-ecosystem/go-grpc-middleware/issues/382

NOTE: I removed tracing package completely since in new PR we complete rewrite it anyway.

Signed-off-by: Bartlomiej Plotka [email protected]

We are updating golang.org/x/net deps to release-branch.go1.11 in Kubernetes but we are facing a problem as go-grpc-middleware still uses the older version of golang.org/x/net package.

Will it be a good idea to update Gopkg.lock so that it uses the latest version of dependencies?

Fixes #346

Some questions:

• [x] Should we integrate Prometheus in the metrics package, or provide it as another provider?

Have Prometheus as a separate provider, which follows Open Metrics convention, we need to decide what do we want to name the package though since the repo is Open Metrics compliant

• [x] How should we add in options for enabling the histograms for different purposes?

This PR is a WIP PR, however suggestions regarding the implementation details are welcome because most of the structure of the code is final. The interface for the reporter has been tweaked a little for enabling the Timer feature from Prometheus.

cc @bwplotka @brancz

TODO:

• [x] Write tests for the new provider
• [ ] Write docs for the same

This continues https://github.com/grpc-ecosystem/go-grpc-middleware/pull/97 by addressing feedback to replace RequestFieldExtractorFunc with RequestFieldExtractorWithContextFunc.

cc @graphaelli

Descriptions:

This PR improve the rate-limit interceptor changing interface.

Links:

This PR is follow-up of discussion at https://github.com/thanos-io/thanos/pull/3654

Current issue - peer.address is duplicated in the zap logger middleware:

{"level":"info","ts":1538680314.2086248,"logger":"grpc","caller":"zap/server_interceptors.go:40","msg":"finished unary call with code OK","peer.address":"127.0.0.1:50180","grpc.start_time":"2018-10-04T12:11:54-07:00","system":"grpc","span.kind":"server","grpc.service":"pinpoint.PinpointCore","grpc.method":"GetStatus","peer.address":"127.0.0.1:50180","grpc.code":"OK","grpc.time":0.000100797}

Closes #146

Logging payloads of requests on the server-side and similarly of responses on the client-side is a pretty common and desired use-case. However, logging by default payloads of both requests as well as responses in both scenarios is IMO not a very common requirement.

Consider the following PR to enable more granular control of what to log and when. This is achieved by using a special PayloadDecison type having the following values:

• NoPayloadLogging - Payload logging is disabled.
• LogRequest - Only logging of requests is enabled.
• LogResponse - Only logging of responses is enabled.
• LogRequestAndResponse - Logging of both requests and responses is enabled. The PayloadDecison is to be used instead of the original bool by the logging.ServerPayloadLoggingDecider and logging.ClientPayloadLoggingDecider.

NOTE: I quickly put this together to see whether it could be considered a valid approach, thus please excuse the lack of additional unit tests. I will be glad to add those once the approach is approved.

The commands I ran:

go get golang.org/x/oauth2
go get golang.org/x/oauth2/[email protected]
go mod tidy

Not sure if this is going to be right, but I'm trying to fix the problem in #515 I want to use the grpc_auth middleware, but can't due to outdated dependencies.

I saw v2 changelog says:

tags removed. Use logging.ExtractFields to read logging fields from logging interceptor for your local request logger. Use logging.InjectFields to inject custom fields to logging interceptor to client context or interceptor before logging interceptor.

In the v2 readme demo, it still uses old tags:

import "github.com/grpc-ecosystem/go-grpc-middleware/v2"

myServer := grpc.NewServer(
    grpc.ChainStreamInterceptor(
        tags.StreamServerInterceptor(),
        opentracing.StreamServerInterceptor(),
        prometheus.StreamServerInterceptor,
        zap.StreamServerInterceptor(zapLogger),
        auth.StreamServerInterceptor(myAuthFunction),
        recovery.StreamServerInterceptor(),
    ),
    grpc.ChainUnaryInterceptor(
        tags.UnaryServerInterceptor(),
        opentracing.UnaryServerInterceptor(),
        prometheus.UnaryServerInterceptor,
        zap.UnaryServerInterceptor(zapLogger),
        auth.UnaryServerInterceptor(myAuthFunction),
        recovery.UnaryServerInterceptor(),
    ),
)

It would be great to update readme for how to use logging.ExtractFields. Thanks! 😃

We already have rate limit interceptor at server side after #134. I'm considering to have one at client side to protect target service if server, since server may have no rate limit at all.

During migration to v2 we are expiring an issue that we cannot use nested or other types than string as custom log field value. We previously used ctxzap to add a map[string]string as log field value.

Current implementation using v1

labels := map[string]string{
	"traceId": "insert-trace-id-here",
}
// wont compile because MarshalLogObject not implemented, should only serve as demonstration
ctxzap.AddFields(ctx, zap.Object("logging.googleapis.com/labels", labelValues))

We than expect the log to contain the following

{
  "logging.googleapis.com/labels": {
    "traceId": "insert-trace-id-here"
  }
}

As zap does not use fields from ctxzap anymore we had to migrate to logging.InjectFields which leads to the following implementation

type LogFieldValue interface {
	String() string
}

func InjectLogField(parentContext context.Context, key string, value LogFieldValue) context.Context {
	return logging.InjectFields(parentContext, logging.Fields{key, value.String()})
}

The problem with the new InjectFields method is that logging.Fields only allows string values so we cannot pass the nested data how we did before. The generated log now looks like this (using JSON Marshal)

{
  "logging.googleapis.com/labels": "{\"traceId\":\"insert-trace-id-here\"}"
}

Is there any way we can archive to get the same output log as in v1 using the v2? If not, should we change the type of logging.Fields to accept more than just strings as value?

Thanks in advance