Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

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

Merged
merged 5 commits into from
Apr 19, 2019
Merged

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

merged 5 commits into from
Apr 19, 2019

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm 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

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 changed the title [WIP] *: use pingcap/log (zap) for logging *: use pingcap/log (zap) for logging Apr 16, 2019
@kennytm
Copy link
Collaborator Author

kennytm 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 April 16, 2019 18:56
@kennytm kennytm added priority/important status/PTAL This PR is ready for review. Add this label back after committing new changes type/enhancement Performance improvement or refactoring labels Apr 16, 2019
@kennytm
Copy link
Collaborator Author

kennytm 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
Copy link
Collaborator Author

kennytm commented Apr 18, 2019

/run-all-tests

@IANTHEREAL
Copy link
Collaborator

@july2993 @lonng PTAL

Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc added status/LGT1 One reviewer already commented LGTM (LGTM1) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Apr 19, 2019
Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

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

Rest LGTM

@kennytm
Copy link
Collaborator Author

kennytm commented Apr 19, 2019

/run-all-tests

Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

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

LGTM

@july2993 july2993 added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Apr 19, 2019
@kennytm kennytm merged commit a515465 into master Apr 19, 2019
@kennytm kennytm deleted the kennytm/pingcap-log-3 branch April 25, 2019 06:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) type/enhancement Performance improvement or refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants