stepancheg / grpc-rust

Rust implementation of gRPC
MIT License
1.37k stars 124 forks source link

Replace info logs with debug logs #149

Closed FlyingRatBull closed 5 years ago

FlyingRatBull commented 5 years ago

Replacing the info logs with debug logs allows the usage in production environments with heavy API usage without cluttering the logs.

stepancheg commented 5 years ago

Should start_call be left INFO?

Generally, after startup which messages should be INFO or higher?

FlyingRatBull commented 5 years ago

I think as gRPC is a tool used in an application and not the main purpopse of an application that no message should be INFO. If it makes sense for an application to notify the user about gRPC startup or any other gRPC related event, it (the application) should do so by itself. But almost all the time the application is supposed to be handling any events like startups or even errors.

In other gRPC implementations there are commonly one of two approaches:

stepancheg commented 5 years ago

OK, but what about events like:

zirpu commented 5 years ago

OK, but what about events like:

* client connection accepted on the server

* reconnect happened on the client

* network error

* protocol error
  they also should be DEBUG?

I would argue that those should default to DEBUG because the are out of the server's control. One would only turn on logging to debug the issue, not as a production log.

Though I do acknowledge that collecting metrics of those errors would be useful, filling up a log file with them would be useless in a non-debuging context.

stepancheg commented 5 years ago

Honestly, I never had a love for logs. Currently, different levels are useful for debugging: with env logger different logging levels are displayed in different colours.

So I have no preferences what should be logging levels for which events.

Anyway, what you say makes sense. Before applying it let's specify what events should be at which level:

This looks consistent, but usually, log filtering can be used to hide certain events. E. g. if you are not interested in request started event or connection open, you hide INFO level.

Anyway, as I said, I don't have strong preference, but I'd like to have a semi-formal description.

FlyingRatBull commented 5 years ago

That sounds quite good to me. Maybe in the future we can pass a logger to the client/server so the user can decide by himself, how to handle the logs or disable them completely.

stepancheg commented 5 years ago

Maybe in the future we can pass a logger to the client/server so the user can decide by himself, how to handle the logs or disable them completely

To avoid misunderstanding: users currently can configure the log level. E. g. if using env_logger, it can be something like

RUST_LOG=grpc=warn myexe

The discussion is about what levels should be the default.

Or I did not get your point.

FlyingRatBull commented 5 years ago

I meant 'user' more in the sense of the developer, using the gRPC implementation in an application. Because it depends not only on the user (which can use the env variable as pointed out by you) but also on the application itself, whether gRPC related logging is relevant or to what extent.

And yes, the discussion was about the default log levels.

stepancheg commented 5 years ago

Thanks!