Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util/logutil: Remove useless grpc log in production #25454

Merged
merged 15 commits into from
Aug 16, 2021
Merged
31 changes: 25 additions & 6 deletions util/logutil/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ const (
DefaultRecordPlanInSlowLog = 1
// DefaultTiDBEnableSlowLog enables TiDB to log slow queries.
DefaultTiDBEnableSlowLog = true
// GRPCLogDebugVerbosity enables max verbosity when debugging grpc code.
GRPCLogDebugVerbosity = 99
)

// EmptyFileLogConfig is an empty FileLogConfig.
Expand Down Expand Up @@ -111,12 +109,33 @@ func InitLogger(cfg *LogConfig) error {
return errors.Trace(err)
}

// init logger for grpc debugging
err = initGRPCLogger(cfg)
if err != nil {
return errors.Trace(err)
}

return nil
}

func initGRPCLogger(cfg *LogConfig) error {
// copy Config struct by assignment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// copy Config struct by assignment
// Copy Config struct by assignment.

config := cfg.Config
// hack: force stdout
config.File.Filename = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to force output grpc log to stdout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old logic directs all grpc log to std. Please take a look at

if len(os.Getenv("GRPC_DEBUG")) > 0 {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Then there is a problem: the old logic output to stderr. In the tiup deployment, stderr will be piped to the tidb_stderr.log file eventually. But if you redirect them to the stdout as in this PR, then you will just lose your outputs, as stdout is not collected anywhere..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you find it hard to print to stderr then I think it is fine to print in the log file. There is no much difference from my perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

if len(os.Getenv("GRPC_DEBUG")) > 0 {
// more information for verbosity: https://github.com/google/glog#verbose-logging
gzap.ReplaceGrpcLoggerV2WithVerbosity(gl, GRPCLogDebugVerbosity)
config.Level = "debug"
l, _, err := log.InitLogger(&cfg.Config, zap.AddStacktrace(zapcore.DebugLevel))
if err != nil {
return errors.Trace(err)
}
gzap.ReplaceGrpcLoggerV2WithVerbosity(l, 999)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change the value of verbosity?

Copy link
Contributor Author

@SabaPing SabaPing Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpclog.SetLoggerV2(grpclog.NewLoggerV2WithVerbosity(os.Stderr, os.Stderr, os.Stderr, 999))

The original logic set the verbosity to 999. I just migrated the code and kept it unchanged.

} else {
gzap.ReplaceGrpcLoggerV2(gl)
config.Level = "error"
l, _, err := log.InitLogger(&cfg.Config, zap.AddStacktrace(zapcore.FatalLevel))
if err != nil {
return errors.Trace(err)
}
gzap.ReplaceGrpcLoggerV2(l)
}

return nil
Expand Down
6 changes: 3 additions & 3 deletions util/logutil/slow_query_logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ var _pool = buffer.NewPool()

func newSlowQueryLogger(cfg *LogConfig) (*zap.Logger, error) {

// reuse global config and override slow query log file
// copy global config and override slow query log file
// if slow query log filename is empty, slow query log will behave the same as global log
sqConfig := &cfg.Config
sqConfig := cfg.Config
if len(cfg.SlowQueryFile) != 0 {
sqConfig.File = log.FileLogConfig{
MaxSize: cfg.File.MaxSize,
Expand All @@ -26,7 +26,7 @@ func newSlowQueryLogger(cfg *LogConfig) (*zap.Logger, error) {
}

// create the slow query logger
sqLogger, prop, err := log.InitLogger(sqConfig)
sqLogger, prop, err := log.InitLogger(&sqConfig)
if err != nil {
return nil, errors.Trace(err)
}
Expand Down