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

SabaPing · 2021-06-15T13:47:29Z

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

Need to cherry-pick to the release branch

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

Release note

No release note

sre-bot · 2021-06-15T13:48:00Z

Please follow PR Title Format:

pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

*: what's changed

ti-srebot · 2021-06-15T13:48:00Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

SabaPing · 2021-06-15T13:51:53Z

/rebuild

util/logutil/log.go

ti-chi-bot · 2021-06-16T09:45:53Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

breeswish
zimulala

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

breezewish · 2021-06-16T09:46:10Z

util/logutil/log.go

+	// copy Config struct by assignment
+	config := cfg.Config
+	// hack: force stdout
+	config.File.Filename = ""


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

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

tidb/tidb-server/main.go

Line 592 in 4f31cb4

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

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

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.

breezewish · 2021-06-17T04:57:17Z

util/logutil/log_test.go

+	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)


how about aseerting that after calling the function the original config's level remains unchanged?

zimulala · 2021-06-17T06:20:30Z

util/logutil/log.go

+		if err != nil {
+			return nil, nil, errors.Trace(err)
+		}
+		gzap.ReplaceGrpcLoggerV2WithVerbosity(l, 999)


Why change the value of verbosity?

tidb/tidb-server/main.go

Line 593 in 4f31cb4

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.

zimulala · 2021-07-29T13:41:32Z

util/logutil/log.go

+}
+
+func initGRPCLogger(cfg *LogConfig) (*zap.Logger, *log.ZapProperties, error) {
+	// copy Config struct by assignment


Suggested change

// copy Config struct by assignment

// Copy Config struct by assignment.

SabaPing · 2021-08-12T13:46:49Z

I'll pick up this issue tomorrow.

SabaPing · 2021-08-13T05:35:34Z

PTAL @tisonkun @breeswish @zimulala

zimulala

LGTM

breezewish · 2021-08-15T11:47:18Z

/merge

ti-chi-bot · 2021-08-15T11:47:22Z

This pull request has been accepted and is ready to merge.

Commit hash: 19660f4

breezewish · 2021-08-16T04:57:40Z

/merge

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-08-16T05:10:50Z

cherry pick to release-5.1 in PR #27238

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-08-16T05:11:57Z

cherry pick to release-5.2 in PR #27239

remove useless grpc log

c1cc79a

ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 15, 2021

SabaPing changed the title ~~[util/log]Remove useless grpc log in production~~ util/log: Remove useless grpc log in production Jun 15, 2021

SabaPing changed the title ~~util/log: Remove useless grpc log in production~~ util/logutil: Remove useless grpc log in production Jun 15, 2021

SabaPing added 2 commits June 15, 2021 22:15

use deepcopy to make init more safer

6c76acc

fix null pointer

6062f29

SabaPing force-pushed the remove-grpc-log branch from 0bae5bd to 6062f29 Compare June 15, 2021 14:32

breezewish reviewed Jun 15, 2021

View reviewed changes

util/logutil/log.go Show resolved Hide resolved

breezewish added the needs-cherry-pick-release-5.1 label Jun 15, 2021

SabaPing added 2 commits June 16, 2021 17:28

Merge remote-tracking branch 'ups/master' into remove-grpc-log

7054a6a

fix config copy

af74b3d

breezewish approved these changes Jun 16, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 16, 2021

breezewish reviewed Jun 16, 2021

View reviewed changes

breezewish added the type/bugfix This PR fixes a bug. label Jun 16, 2021

add unit test

66e4c6c

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 17, 2021

grpc log follows global log output

ac14a4e

SabaPing force-pushed the remove-grpc-log branch from 5c2bf7a to ac14a4e Compare June 17, 2021 04:53

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 17, 2021

breezewish approved these changes Jun 17, 2021

View reviewed changes

assert after init a logger, the original conf is not changed

fcf78d1

zimulala reviewed Jun 17, 2021

View reviewed changes

breezewish approved these changes Jun 17, 2021

View reviewed changes

zimulala reviewed Jul 29, 2021

View reviewed changes

SabaPing added 3 commits August 13, 2021 10:21

Merge branch 'master' into remove-grpc-log

26a8776

migrate test framework to testify

ea31a51

fix comments

19660f4

zimulala approved these changes Aug 13, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 13, 2021

breezewish added the needs-cherry-pick-release-5.2 label Aug 15, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 15, 2021

ti-chi-bot added 3 commits August 15, 2021 19:47

Merge branch 'master' into remove-grpc-log

3f64c13

Merge branch 'master' into remove-grpc-log

b95ee6c

Merge branch 'master' into remove-grpc-log

d653e11

Merge branch 'master' into remove-grpc-log

eea4682

ti-chi-bot merged commit ed88b54 into pingcap:master Aug 16, 2021

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 16, 2021

cherry pick pingcap#25454 to release-5.1

56afa35

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Aug 16, 2021

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

Merged

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 16, 2021

cherry pick pingcap#25454 to release-5.2

ed03750

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Aug 16, 2021

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

Merged

SabaPing deleted the remove-grpc-log branch August 31, 2021 06:24

SabaPing mentioned this pull request Aug 31, 2021

Tracking issue for unifying log library #24190

Open

7 tasks

ti-chi-bot pushed a commit that referenced this pull request Sep 17, 2021

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

dc07933

ti-chi-bot pushed a commit that referenced this pull request Oct 14, 2021

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

21075bb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SabaPing commented Jun 15, 2021 •

edited

Loading

sre-bot commented Jun 15, 2021

ti-srebot commented Jun 15, 2021

SabaPing commented Jun 15, 2021

ti-chi-bot commented Jun 16, 2021 •

edited

Loading

breezewish Jun 16, 2021

SabaPing Jun 16, 2021

breezewish Jun 16, 2021

breezewish Jun 16, 2021

SabaPing Jun 17, 2021

breezewish Jun 17, 2021

SabaPing Jun 17, 2021

zimulala Jun 17, 2021

SabaPing Jun 17, 2021 •

edited

Loading

zimulala Jul 29, 2021

SabaPing commented Aug 12, 2021

SabaPing commented Aug 13, 2021

zimulala left a comment

breezewish commented Aug 15, 2021

ti-chi-bot commented Aug 15, 2021

breezewish commented Aug 16, 2021

ti-srebot commented Aug 16, 2021

ti-srebot commented Aug 16, 2021

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

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

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

Conversation

SabaPing commented Jun 15, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

sre-bot commented Jun 15, 2021

ti-srebot commented Jun 15, 2021

SabaPing commented Jun 15, 2021

ti-chi-bot commented Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SabaPing Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SabaPing commented Aug 12, 2021

SabaPing commented Aug 13, 2021

zimulala left a comment

Choose a reason for hiding this comment

breezewish commented Aug 15, 2021

ti-chi-bot commented Aug 15, 2021

breezewish commented Aug 16, 2021

ti-srebot commented Aug 16, 2021

ti-srebot commented Aug 16, 2021

SabaPing commented Jun 15, 2021 •

edited

Loading

ti-chi-bot commented Jun 16, 2021 •

edited

Loading

SabaPing Jun 17, 2021 •

edited

Loading