util/logutil: Remove useless grpc log in production #25454
Conversation
ti-chi-bot
added
the
size/M
|
Please follow PR Title Format:
Or if the count of mainly changed packages are more than 3, use
|
|
No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md |
SabaPing
changed the title
SabaPing
changed the title
|
/rebuild |
SabaPing
force-pushed
the
remove-grpc-log
branch
from
0bae5bd to
6062f29
Compare
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
util/logutil/log.go
Outdated
| // copy Config struct by assignment | ||
| config := cfg.Config | ||
| // hack: force stdout | ||
| config.File.Filename = "" |
There was a problem hiding this comment.
Why do we need to force output grpc log to stdout?
There was a problem hiding this comment.
The old logic directs all grpc log to std. Please take a look at
Line 592 in 4f31cb4
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
ti-chi-bot
added
size/L
SabaPing
force-pushed
the
remove-grpc-log
branch
from
5c2bf7a to
ac14a4e
Compare
ti-chi-bot
added
size/M
util/logutil/log_test.go
Outdated
| conf := NewLogConfig("info", DefaultLogFormat, "", EmptyFileLogConfig, false) | ||
| copiedConfig := *conf | ||
| // assert that the new conf's address not same as the old one | ||
| c.Assert(&copiedConfig != conf, Equals, true) |
There was a problem hiding this comment.
how about aseerting that after calling the function the original config's level remains unchanged?
| if err != nil { | ||
| return nil, nil, errors.Trace(err) | ||
| } | ||
| gzap.ReplaceGrpcLoggerV2WithVerbosity(l, 999) |
There was a problem hiding this comment.
Why change the value of verbosity?
There was a problem hiding this comment.
Line 593 in 4f31cb4
The original logic set the verbosity to 999. I just migrated the code and kept it unchanged.
util/logutil/log.go
Outdated
| } | ||
|
|
||
| func initGRPCLogger(cfg *LogConfig) (*zap.Logger, *log.ZapProperties, error) { | ||
| // copy Config struct by assignment |
There was a problem hiding this comment.
| // copy Config struct by assignment | |
| // Copy Config struct by assignment. |
|
I'll pick up this issue tomorrow. |
ti-chi-bot
added
status/LGT2
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 19660f4
|
ti-chi-bot
added
the
status/can-merge
|
/merge |
Signed-off-by: ti-srebot <[email protected]>
|
cherry pick to release-5.1 in PR #27238 |
Signed-off-by: ti-srebot <[email protected]>
|
cherry pick to release-5.2 in PR #27239 |
What problem does this PR solve?
There is a pr #23534, refactors the log code. It rewrites the initialization logic of the grpc logger, resulting in annoying grpc logs.
This pr remove those useless logs.
What is changed and how it works?
What's Changed:
Refactor the grpc logger initialization logic.
Related changes
#23534
Check List
Tests
Release note