Proposal: Unify Log Library for Golang Projects #24181
Conversation
ti-chi-bot
added
the
size/L
|
No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md |
|
No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md |
|
@tisonkun: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
@tisonkun: In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
| For historical reasons, TiDB has two third-party logging libraries, `logrus` and `pingcap/log`. `pingcap/log` is a wrapper of `zap`. | ||
|
|
||
| Logs of TiDB can be divided into two types, slow query logs and the other logs. | ||
| As mentioned above, these two types of logs are emitted through two different logging libraries, which results in separate configurations for the two types of logs and requires writing additional configuration conversion code. |
There was a problem hiding this comment.
These two logs are intentionally to have different formats. Will there be a problem when merging the two separated configuration into one?
There was a problem hiding this comment.
I added extra conversion layers. After testing and verifying, it should be fine.
Co-authored-by: Wenxuan <[email protected]>
I've refactored doc following the standard proposal format. |
Co-authored-by: tison <[email protected]>
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
ti-chi-bot
added
status/LGT2
|
/assign @zz-jason |
|
/assign @bb7133 |
| Implementation plan: | ||
|
|
||
| 1. Unify log library for `pingcap/tidb` first. For dependent parts we write dummy code to satisfy. | ||
| 2. Unify log library for `pingcap/br`, remove the dependency on `pingcap/tidb/util/logutil`, and clear dummy code of `pingcap/tidb`. |
There was a problem hiding this comment.
How about the eco-system tools like 'TiCDC' and 'Dumpling'?
| Mainly unit testing. | ||
|
|
||
| ## Open issues (if applicable) | ||
| ## Logging code for each component |
|
|
||
| There must be something wrong with the engineering of these codes above, and they must be changed. The cost to change them is not small. | ||
|
|
||
| Rationale - for long-term consideration, we should maintain code quality. The speed of output can be sacrificed in time if necessary. |
There was a problem hiding this comment.
The speed of output can be sacrificed in time if necessary.
I guess you're talking about the 'performance', or more precisely, 'maintainability should be with higher priority over performance when we have to consider the trade-offs'.
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: d06fb45
|
ti-chi-bot
added
the
status/can-merge
|
@SabaPing: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
What problem does this PR solve?
The primary goal is to clean up the logging component. TiDB depends on two different third part log components, which are logrus and zap. This creates unnecessary complexity. In addition, there are some other logging flaws in code.
What's Changed?
Add a proposal doc for the whole task.
Release note