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

log,server: add contextual log util & test it in conn.go #9548

Merged
merged 8 commits into from Mar 9, 2019

Conversation

@lysu
Copy link
Member

lysu commented Mar 4, 2019

What problem does this PR solve?

we often forgot to log connId in log content that make us hard to distinguish operations from different session, and manual log it in every log is duplicated.

What is changed and how it works?

  • add a Logger(ctx context.Context) method
  • add connId and recvTs in root ctx and pass them down
  • demo new usage in conn.go

Question

it's harder to modify low layer module's log, because some of our context.Context is miss during some methods, but it's also a question that needs be addressed, tracing also depends on context.Context passing

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • change log

Side effects

  • N/A

Related changes

  • Need to cherry-pick to the release 2.1

This change is Reviewable

@lysu lysu added this to In Progress in Improve usability via automation Mar 4, 2019

@lysu lysu requested review from zimulala , tiancaiamao and zz-jason Mar 4, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 4, 2019

Codecov Report

Merging #9548 into master will decrease coverage by 0.031%.
The diff coverage is 37.8378%.

@@               Coverage Diff                @@
##             master      #9548        +/-   ##
================================================
- Coverage   67.4009%   67.3699%   -0.0311%     
================================================
  Files           374        374                
  Lines         78895      78921        +26     
================================================
- Hits          53176      53169         -7     
- Misses        20966      20997        +31     
- Partials       4753       4755         +2
Show resolved Hide resolved util/logutil/log.go Outdated
Show resolved Hide resolved server/conn.go Outdated
Show resolved Hide resolved server/conn.go Outdated
Show resolved Hide resolved server/conn.go Outdated
Show resolved Hide resolved server/conn.go Outdated
Show resolved Hide resolved server/conn.go Outdated
Show resolved Hide resolved server/conn.go Outdated
Show resolved Hide resolved util/logutil/log.go Outdated
Show resolved Hide resolved util/logutil/log.go Outdated
Show resolved Hide resolved util/logutil/log.go Outdated
Show resolved Hide resolved util/logutil/log.go Outdated
Show resolved Hide resolved server/conn.go Outdated
Show resolved Hide resolved server/conn.go Outdated

@lysu lysu force-pushed the lysu:dev-ctx-log branch from 75d501d to bbe955f Mar 6, 2019

Show resolved Hide resolved server/conn.go Outdated
@zz-jason
Copy link
Member

zz-jason left a comment

LGTM

@lysu lysu added the status/LGT1 label Mar 7, 2019

@lysu lysu requested a review from jackysp Mar 7, 2019

@lysu lysu force-pushed the lysu:dev-ctx-log branch from a9c196a to fc6a76e Mar 7, 2019

@lysu

This comment has been minimized.

Copy link
Member Author

lysu commented Mar 7, 2019

/run-all-tests

@lysu lysu force-pushed the lysu:dev-ctx-log branch from fc6a76e to 9b0578f Mar 8, 2019

Show resolved Hide resolved server/conn.go Outdated
logutil.Logger(ctx).Warn("dispatch error",
zap.String("connInfo", cc.String()),
zap.String("sql", queryStrForLog(string(data[1:]))),
zap.String("err", errStrForLog(err)),

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Mar 8, 2019

Contributor

zap.Error(err) ?

This comment has been minimized.

@lysu

lysu Mar 8, 2019

Author Member

maybe better keep errStrForLog , it seems errStrForLog in purpose to suspend stack info for duplicate key error

if kv.ErrKeyExists.Equal(err) {

@jackysp
Copy link
Member

jackysp left a comment

LGTM

@jackysp

jackysp approved these changes Mar 9, 2019

@jackysp jackysp merged commit 4188ae1 into pingcap:master Mar 9, 2019

7 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 37.8378% of diff hit (target 0%)
Details
codecov/project 67.3699% (-0.0311%) compared to cb7a624
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

Improve usability automation moved this from In Progress to Done Mar 9, 2019

@lysu lysu deleted the lysu:dev-ctx-log branch Mar 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.