grpc-ecosystem / go-grpc-middleware

Golang gRPC Middlewares: interceptor chaining, auth, logging, retries and more.
Apache License 2.0
6.26k stars 690 forks source link

[logging] disable option does not work with request/response fields #679

Closed frankgreco closed 10 months ago

frankgreco commented 10 months ago

Go version used:

go1.21.3

What happened:

With the following configuration, I do not see these fields ignored.

logOpts := []grpc_logging.Option{
  grpc_logging.WithLogOnEvents(
    grpc_logging.StartCall,
    grpc_logging.FinishCall,
    grpc_logging.PayloadReceived,
    grpc_logging.PayloadSent,
  ),
  grpc_logging.WithDisableLoggingFields("grpc.response.content", "grpc.request.content"),
}

What you expected to happen:

I expect them to be ignored.

How to reproduce it (as minimally and precisely as possible):

Use that above config.

johanbrandhorst commented 10 months ago

Hi. You need to use one of the constants as options: https://github.com/grpc-ecosystem/go-grpc-middleware/pull/631/files#diff-8c12c7fac52459b44598f215bb359e03c4ac3678de743b90e4a88c9ad9b03272R42-R47. It isn't a general purpose log disabling option. We could definitely do with some better documentation here. CC @coleenquadros

johanbrandhorst commented 10 months ago

Some better docs have been added to this method, hopefully that will avoid confusion around this in the future.

frankgreco commented 10 months ago

What would be the recommended way to accomplish what I'm trying to accomplish?

johanbrandhorst commented 10 months ago

It's not possible today AFAIK, but maybe it could be added if it makes sense? I often recommend users just fork if they have very specialized needs, but if you think it makes sense as a general purpose option we could consider that too.

frankgreco commented 10 months ago

Good to know. I think this use case would be pretty common, "i want to know every time a message is sent but I don't want to log the request or response as it could contain PII"

Would we re-open this as a feature request or a new issue?

johanbrandhorst commented 10 months ago

I'd consider it a separate issue, if you don't mind. If we could tie it to the existing option that would be great too.