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.

Sign up for GitHub

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

Jump to bottom

domain,owner: unify and normalize the format of the log #9646

Merged
merged 60 commits into from
Mar 25, 2019
Merged

domain,owner: unify and normalize the format of the log #9646

merged 60 commits into from
Mar 25, 2019

Conversation

xiekeyi98
Copy link
Contributor

What problem does this PR solve?

Unify and normalize the format of the log in the pkg of domain and owner.

What is changed and how it works?

Change log "github.com/sirupsen/logrus" to github.com/pingcap/log.

Check List

Tests

  • Unit test

@xiekeyi98 xiekeyi98 added the type/enhancement The issue or PR belongs to an enhancement. label Mar 11, 2019
xiekeyi98
xiekeyi98 commented Mar 11, 2019
View reviewed changes
owner/manager.go Outdated
@@ -155,7 +156,7 @@ func NewSession(ctx context.Context, logPrefix string, etcdCli *clientv3.Client,
break
}
if failedCnt%logIntervalCnt == 0 {
log.Warnf("%s failed to new session to etcd, err %v", logPrefix, err)
log.Warn("Failed to new session to etcd", zap.String("logPrefix", logPrefix), zap.Error(err))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what I do is correct.
How to solve the variable logPrefix?

owner/manager.go Outdated
@@ -189,7 +190,7 @@ func (m *ownerManager) ResignOwner(ctx context.Context) error {
return errors.Trace(err)
}

log.Warnf("%s Resign ddl owner success!", m.logPrefix)
log.Warn("Resign ddl owner success!", zap.String("logPrefix", m.logPrefix))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

s/Warn/Info?
I don't understand why this log is Warning.

owner/manager.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 11, 2019 •
edited
Loading

Codecov Report

Merging #9646 into master will decrease coverage by 0.0062%.
The diff coverage is 22.093%.

@@               Coverage Diff                @@
##             master      #9646        +/-   ##
================================================
- Coverage   67.2941%   67.2878%   -0.0063%     
================================================
  Files           385        385                
  Lines         80588      80600        +12     
================================================
+ Hits          54231      54234         +3     
- Misses        21502      21508         +6     
- Partials       4855       4858         +3

zimulala
zimulala reviewed Mar 14, 2019
View reviewed changes
domain/domain.go Outdated Show resolved Hide resolved
domain/domain.go Outdated Show resolved Hide resolved
@xiekeyi98
Copy link
Contributor Author

Because of the weekly meeting discussed, this PR will work in progress for logutil.

xiekeyi98
xiekeyi98 commented Mar 15, 2019
View reviewed changes
domain/domain.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 reviewed Mar 19, 2019
View reviewed changes
domain/domain.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 reviewed Mar 19, 2019
View reviewed changes
domain/domain.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 reviewed Mar 19, 2019
View reviewed changes
domain/domain.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 reviewed Mar 19, 2019
View reviewed changes
domain/domain.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 reviewed Mar 19, 2019
View reviewed changes
domain/domain.go Outdated Show resolved Hide resolved
winkyao
winkyao reviewed Mar 23, 2019
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment •
edited
Loading

Choose a reason for hiding this comment

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

reset LGTM

domain/domain.go Show resolved Hide resolved
domain/domain.go Outdated Show resolved Hide resolved
zimulala
zimulala reviewed Mar 23, 2019
View reviewed changes
domain/domain.go Outdated Show resolved Hide resolved
domain/domain.go Show resolved Hide resolved
domain/schema_validator.go Outdated Show resolved Hide resolved
domain/schema_validator.go Outdated Show resolved Hide resolved
domain/schema_validator.go Outdated Show resolved Hide resolved
owner/manager.go Outdated Show resolved Hide resolved
owner/manager.go Outdated Show resolved Hide resolved
owner/manager.go Outdated Show resolved Hide resolved
domain/domain.go Outdated Show resolved Hide resolved
domain/domain.go Outdated Show resolved Hide resolved
@xiekeyi98
Copy link
Contributor Author

@zimulala
As the doc says, I think Any is better than Reflect.
Because :
It uses an encoding-appropriate, reflection-based function to lazily serialize nearly any object into the logging context, but it's relatively slow and allocation-heavy. Outside tests, Any is always a better choice.

Any takes a key and an arbitrary value and chooses the best way to represent them as a field, falling back to a reflection-based approach only if necessary.
Since byte/uint8 and rune/int32 are aliases, Any can't differentiate between them. To minimize surprises, []byte values are treated as binary blobs, byte values are treated as uint8, and runes are always treated as integers.

Reflect constructs a field with the given key and an arbitrary object. It uses an encoding-appropriate, reflection-based function to lazily serialize nearly any object into the logging context, but it's relatively slow and allocation-heavy. Outside tests, Any is always a better choice.
If encoding fails (e.g., trying to serialize a map[int]string to JSON), Reflect includes the error message in the final log output.

@zimulala
Copy link
Contributor

zimulala commented Mar 25, 2019 •
edited
Loading

@xiekeyi98
If we know that it can't be handled with a known type, I think it's possible to use Reflect directly. The recover function is where I got it wrong.

@zimulala zimulala added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Mar 25, 2019
@xiekeyi98 xiekeyi98 requested a review from crazycs520 March 25, 2019 04:21
zimulala
zimulala reviewed Mar 25, 2019
View reviewed changes
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala
Copy link
Contributor

/run-all-tests

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 25, 2019
@xiekeyi98
Copy link
Contributor Author

/run-all-tests

@xiekeyi98
Copy link
Contributor Author

/run-common-test

winkyao
winkyao approved these changes Mar 25, 2019
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@winkyao
Copy link
Contributor

winkyao commented Mar 25, 2019

/run-common-test

@xiekeyi98
Copy link
Contributor Author

/run-common-test -tidb-test=pr/763

@xiekeyi98
Copy link
Contributor Author

/run-common-test tidb-test=pr/763

@zz-jason zz-jason merged commit 1154456 into pingcap:master Mar 25, 2019
@xiekeyi98 xiekeyi98 deleted the TiDB-3014 branch March 25, 2019 08:55
This was referenced Mar 29, 2019
Merged
Merged
zz-jason pushed a commit that referenced this pull request Apr 4, 2019
@xiekeyi98 @zz-jason
domain,owner: unify and normalize the format of the log (#9646)(#10012)…
1dba727 
… (#9953)
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimulala zimulala zimulala left review comments

@morgo morgo morgo left review comments

@winkyao winkyao winkyao approved these changes

@crazycs520 crazycs520 Awaiting requested review from crazycs520

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. priority/release-blocker This issue blocks a release. Please solve it ASAP. status/LGT1 Indicates that a PR has LGTM 1. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.