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

Feat: add zap log print #230

Closed
wants to merge 1 commit into from
Closed

Conversation

fengshunli
Copy link
Member

No description provided.

@@ -1,3 +1,4 @@
//go:build debug
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need

Copy link
Member Author

Choose a reason for hiding this comment

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

go fmt

@@ -1,3 +1,4 @@
//go:build !debug
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

go fmt

Comment on lines 128 to 132
Filename: logpath, // 日志文件的位置
MaxSize: 10, // 在进行切割之前,日志文件的最大大小(以MB为单位)
MaxBackups: 200, // 保留旧文件的最大个数
MaxAge: 30, // 保留旧文件的最大天数
Compress: true, // 是否压缩/归档旧文件
Copy link
Collaborator

Choose a reason for hiding this comment

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

English

Copy link
Member Author

Choose a reason for hiding this comment

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

done

log.Field("LogPath", logpath),
log.Field("LogLevel", config.GetLogLevel()))
}
logpath := fmt.Sprintf("%s/curveadm.log", curveadm.logDir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the date-related information removed from the log name?

Copy link
Member Author

Choose a reason for hiding this comment

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

The content of the same file reaches the specified number to be compressed. Passing the date will result in too many small files and cannot be cleared

cli/cli/cli.go Outdated
@@ -148,17 +145,17 @@ func (curveadm *CurveAdm) init() error {
dbpath := fmt.Sprintf("%s/curveadm.db", curveadm.dataDir)
s, err := storage.NewStorage(dbpath)
if err != nil {
log.Error("Init SQLite database failed",
log.Field("Error", err))
zaplog.LOG.Error("Init SQLite database failed",
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can avoid directly exposing the variable LOG by adding functions in zaplog.go, which can also be used in the same way as the original, such as zaplog.Info(), zaplog.Error() and other functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -41,7 +42,7 @@ import (
tui "github.com/opencurve/curveadm/internal/tui/common"
"github.com/opencurve/curveadm/internal/utils"
cliutil "github.com/opencurve/curveadm/internal/utils"
log "github.com/opencurve/curveadm/pkg/log/glg"
"github.com/opencurve/curveadm/pkg/log/zaplog"
Copy link
Collaborator

@Cyber-SiKu Cyber-SiKu May 19, 2023

Choose a reason for hiding this comment

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

Suggested change
"github.com/opencurve/curveadm/pkg/log/zaplog"
log "github.com/opencurve/curveadm/pkg/log/zaplog"

This can keeps your changes small.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Signed-off-by: fengshunli <[email protected]>
@Cyber-SiKu
Copy link
Collaborator

There are some conflicts that can be resolved

@fengshunli
Copy link
Member Author

see #233

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants