Setup logging after reading the config file #856
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Skipping CI for Draft Pull Request. |
Contributor
Author
|
/cc @djshow832 |
Contributor
Author
|
This should probably get some more tests:
|
Contributor
Author
And with the current |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #856 +/- ##
=======================================
Coverage ? 67.09%
=======================================
Files ? 129
Lines ? 12261
Branches ? 0
=======================================
Hits ? 8226
Misses ? 3451
Partials ? 584
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
djshow832
reviewed
Sep 5, 2025
djshow832
approved these changes
Sep 8, 2025
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djshow832 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Collaborator
|
Unit test failed. @dveeden |
|
New changes are detected. LGTM label has been removed. |
Contributor
Author
I'm working on this |
Contributor
Author
|
I haven't figured out why the test is failing exactly. When I run The part of the {
updateCfg: func(cfg *config.LogOnline) {
cfg.LogFile.MaxSize = 3
cfg.LogFile.MaxBackups = 5
},
action: func(log *zap.Logger) {
for range 5 {
msg := strings.Repeat("a", 500*1024)
log.Info(msg)
}
},
check: func(files []os.FileInfo) bool {
if len(files) != 1 {
return false
}
return files[0].Size() >= int64(5*500*1024)
},
},After running the |
Contributor
Author
|
When debugging this I can see that the for look that does the breakpoints:
First bp 1 hit: size = 615554 Second bp 1 hit: size = 1127659 Third bp 1 hit: size = 1639764 Fourth bp 1 hit: size = 2151869 Fifth bp 1 hit: size = 2663974 Hitting bp 2: |
Contributor
Author
|
This shows that it seems to pickup the dvaneeden@dve-carbon:~/dev/pingcap/tiproxy/pkg/manager/logger$ dlv test -- -test.run '^TestUpdateCfg$'
Type 'help' for list of commands.
(dlv) b manager_test.go:69
Breakpoint 1 set at 0xb78535 for github.com/pingcap/tiproxy/pkg/manager/logger.TestUpdateCfg.func5() ./manager_test.go:69
(dlv) c
> [Breakpoint 1] github.com/pingcap/tiproxy/pkg/manager/logger.TestUpdateCfg.func5() ./manager_test.go:69 (hits goroutine(34):1 total:1) (PC: 0xb78535)
64: cfg.LogFile.MaxBackups = 5
65: },
66: action: func(log *zap.Logger) {
67: for range 5 {
68: msg := strings.Repeat("a", 500*1024)
=> 69: log.Info(msg)
70: }
71: },
72: check: func(files []os.FileInfo) bool {
73: if len(files) != 1 {
74: return false
(dlv) p log.core.out.output.Logger.Filename
"/tmp/TestUpdateCfg2325804022/001/proxy.log"
(dlv) p log.core.out.output.Logger.MaxSize
3
(dlv) p log.core.out.output.Logger.MaxBackups
5 |
Contributor
Author
|
Watching the output of This shows that once it starts to do the check, the inode number changes from 4571407 to 4573420 and that So it looks like something closes the old fd and creates a new fd before the check runs. |
Contributor
Author
|
Looks like this makes the test succeed. But it is probably not the right solution. diff --git a/pkg/manager/logger/manager_test.go b/pkg/manager/logger/manager_test.go
index 36d4d3f..e675fd1 100644
--- a/pkg/manager/logger/manager_test.go
+++ b/pkg/manager/logger/manager_test.go
@@ -116,8 +116,10 @@ func TestUpdateCfg(t *testing.T) {
// 2rd will block due to watch channel of size 1
// this ensured all old data are flushed by closing the older file logger
ch <- clonedCfg
+ ch <- clonedCfg
// delete old data after flushed
+ os.Remove(fileName)
err := os.RemoveAll(dir)
require.NoError(t, err)
|
djshow832
reviewed
Nov 7, 2025
| zap.NamedError("cfg marshal error", merr), | ||
| ) | ||
| } | ||
| lm.logger.Info("current config", zap.Any("cfg", acfg)) |
Collaborator
There was a problem hiding this comment.
The test fails because the logger outputs another thing each time the config is updated.
Collaborator
There was a problem hiding this comment.
I suggest you to modify the log level in the test to skip this log. Remember to add a comment to explain this.
Contributor
Author
There was a problem hiding this comment.
I tried this:
diff --git a/pkg/manager/logger/manager_test.go b/pkg/manager/logger/manager_test.go
index 36d4d3f..9562476 100644
--- a/pkg/manager/logger/manager_test.go
+++ b/pkg/manager/logger/manager_test.go
@@ -27,7 +27,7 @@ func TestUpdateCfg(t *testing.T) {
Log: config.Log{
Encoder: "tidb",
LogOnline: config.LogOnline{
- Level: "info",
+ Level: "error",
LogFile: config.LogFile{
Filename: fileName,
MaxSize: 1,And this:
diff --git a/pkg/manager/logger/manager_test.go b/pkg/manager/logger/manager_test.go
index 36d4d3f..54216d6 100644
--- a/pkg/manager/logger/manager_test.go
+++ b/pkg/manager/logger/manager_test.go
@@ -60,6 +60,7 @@ func TestUpdateCfg(t *testing.T) {
},
{
updateCfg: func(cfg *config.LogOnline) {
+ cfg.Level = "error"
cfg.LogFile.MaxSize = 3
cfg.LogFile.MaxBackups = 5
},But neither of these seems to work.
Contributor
Author
There was a problem hiding this comment.
Commenting out the line like this also doesn't work:
diff --git a/pkg/manager/logger/manager.go b/pkg/manager/logger/manager.go
index 97f200c..76c7e76 100644
--- a/pkg/manager/logger/manager.go
+++ b/pkg/manager/logger/manager.go
@@ -77,7 +77,7 @@ func (lm *LoggerManager) watchCfg(ctx context.Context, cfgch <-chan *config.Conf
zap.NamedError("cfg marshal error", merr),
)
}
- lm.logger.Info("current config", zap.Any("cfg", acfg))
+ // lm.logger.Info("current config", zap.Any("cfg", acfg))
}
}
}
|
@dveeden: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #855
Problem Summary:
Logging is setup based with the generic
NewConfig()and used during the initial parsing of the config file. Then later the watcher is correcting things where needed. However the encoder isn't changed. This results in the encoder from the config to be ignored.What is changed and how it works:
As indicated for the changes in
TestChecksum:Check List
Tests
Notable changes
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.