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

*: use pingcap/log (zap) for logging #162

Merged
merged 5 commits into from Apr 19, 2019

Conversation

@kennytm
Copy link
Member

commented Apr 11, 2019

What problem does this PR solve?

Switch to use the uniform log format.

What is changed and how it works?

The log library is changed from logrus to zap, a structured logger.

Due to how structured logging usually works, several interfaces are rewritten.

  • Child loggers are heavily used to abstract away contextual information in the log, e.g. the table name, engine UUID etc. Many types which originally compute these tagging information on-line are changed to storing a child logger instead.
  • The 3 functions QueryRowWithRetry/TransactWithRetry/ExecWithRetry originally writes log without any contextual information. These are modified to accept a logger. The common arguments are abstracted into an SQLWithRetry type.

All logging infrastructures (e.g. common.AppLogger) are moved from common into a new log package.

  • Like common.AppLogger, we need to define our own logger log.L() distinct from the true global one "github.com/pingcap/log".L() so we could control TiDB's logging level independently (though perhaps we could investigate whether merging the two is possible nowadays).
  • Stack traces of error level logs are entirely disabled, and stack traces of errors are by default inhibited by the log.ShortError function, to avoid flooding the log with noise.
  • A common pattern used in Lightning is:
    1. Log that a task has started
    2. Perform a task
    3. Log that a task has ended, and display the total time taken.
      This is codified into the log.Task type.

Several redundant error logs are eliminated (e.g. if f() calls g() calls h(), and h() logs an error, then g() and f() will not log the error again).

Following pingcap/tidb#10025's lead, when refactoring a function, if an error is known to be a custom error, errors.Trace will no longer be applied. This will be done more thoroughly in the future, after unit-test coverage is improved.

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Breaking backward compatibility
    • Log format is modified

Related changes

  • Need to update the documentation
@kennytm kennytm force-pushed the kennytm/pingcap-log-3 branch from 400a63f to 03b305e Apr 12, 2019
Some redundant logs (e.g. logging about the same thing inside and outside
a function) are removed.

The {QueryRow,Transact,Exec}WithRetry functions are revamped to include
the logger.
@kennytm kennytm force-pushed the kennytm/pingcap-log-3 branch from 03b305e to 33759a6 Apr 16, 2019
@kennytm kennytm changed the title [WIP] *: use pingcap/log (zap) for logging *: use pingcap/log (zap) for logging Apr 16, 2019
@kennytm

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

/run-all-tests tidb=master tikv=master pd=master

lightning/common/util.go Outdated Show resolved Hide resolved
@kennytm kennytm marked this pull request as ready for review Apr 16, 2019
@kennytm

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

Output looks like:

Screenshot 2019-04-17 at 02 53 56

lightning/config/config.go Outdated Show resolved Hide resolved
lightning/kv/importer.go Show resolved Hide resolved
lightning/log/log.go Show resolved Hide resolved
lightning/log/log.go Show resolved Hide resolved
lightning/restore/restore.go Show resolved Hide resolved
lightning/restore/restore.go Outdated Show resolved Hide resolved
lightning/restore/restore.go Outdated Show resolved Hide resolved
lightning/restore/restore.go Show resolved Hide resolved
lightning/restore/restore.go Show resolved Hide resolved
lightning/restore/restore.go Outdated Show resolved Hide resolved
kennytm added 2 commits Apr 18, 2019
@kennytm

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

/run-all-tests

@GregoryIan

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

Copy link
Member

left a comment

LGTM

@csuzhangxc csuzhangxc added status/LGT1 and removed status/PTAL labels Apr 19, 2019
Copy link

left a comment

rest LGTM

lightning/verification/checksum.go Show resolved Hide resolved
lightning/restore/restore.go Outdated Show resolved Hide resolved
lightning/restore/restore.go Outdated Show resolved Hide resolved
@lonng
lonng approved these changes Apr 19, 2019
Copy link
Member

left a comment

Rest LGTM

@kennytm

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

/run-all-tests

Copy link

left a comment

LGTM

@july2993 july2993 added status/LGT2 and removed status/LGT1 labels Apr 19, 2019
@kennytm kennytm merged commit a515465 into master Apr 19, 2019
3 checks passed
3 checks passed
idc-jenkins-ci-lightning/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb-lightning/test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@kennytm kennytm deleted the kennytm/pingcap-log-3 branch Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.