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: more precise and detailed handling of errors #111

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mutezebra
Copy link
Member

No description provided.

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 0% with 34 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/base/pack.go 0.00% 19 Missing ⚠️
pkg/cache/paper/set_file_dir.go 0.00% 5 Missing ⚠️
internal/paper/handler.go 0.00% 3 Missing ⚠️
internal/paper/service/get_dir.go 0.00% 3 Missing ⚠️
pkg/cache/paper/get_file_dir.go 0.00% 2 Missing ⚠️
pkg/logger/output.go 0.00% 2 Missing ⚠️
Flag Coverage Δ
unittest 1.01% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/cache/paper/get_file_dir.go 0.00% <0.00%> (ø)
pkg/logger/output.go 0.00% <0.00%> (ø)
internal/paper/handler.go 0.00% <0.00%> (ø)
internal/paper/service/get_dir.go 0.00% <0.00%> (ø)
pkg/cache/paper/set_file_dir.go 0.00% <0.00%> (ø)
pkg/base/pack.go 0.00% <0.00%> (ø)

@@ -47,12 +48,12 @@ func (s *PaperServiceImpl) ListDirFiles(ctx context.Context, req *paper.ListDirF

success, fileDir, err = service.NewPaperService(ctx, s.ClientSet).GetDir(req)
if !success {
logger.Infof("Paper.ListDirFiles: get dir info failed from upyun")
logger.Errorf("Paper.ListDirFiles: get dir info failed from upyun")
Copy link
Member Author

Choose a reason for hiding this comment

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

因为原来的代码并没有对error的内容进行输出,所以这里也没有更改逻辑,只更改了日志等级

resp.Base = base.BuildBaseResp(errors.New("failed to get info from upyun"))
return resp, nil
}
if err != nil {
logger.Infof("Paper.ListDirFiles: get dir info failed: %v", err)
base.LogError(fmt.Errorf("Paper.ListDirFiles: get dir info failed: %w", err))
Copy link
Member Author

Choose a reason for hiding this comment

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

将正常的err中的信息的日志集成进了 base.LogerError中,这里用fmt.Errorf() 对err再包装是因为原来就输出了这些信息,而我不打算更改原有的输出内容

@@ -68,8 +69,7 @@ func (s *PaperServiceImpl) GetDownloadUrl(ctx context.Context, req *paper.GetDow

url, err := service.NewPaperService(ctx, s.ClientSet).GetDownloadUrl(req)
if err != nil {
logger.Infof("Paper.GetDownloadUrl: get download url failed: %v", err)
resp.Base = base.BuildBaseResp(err)
resp.Base = base.BuildRespAndLog(fmt.Errorf("Paper.GetDownloadUrl: get download url failed: %w", err))
Copy link
Member Author

Choose a reason for hiding this comment

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

这里是另一种日志输出err中信息的方法,他将日志与resp的build集成在了一起,减轻开发者心智负担

@@ -49,6 +49,9 @@ func (s *PaperService) GetDir(req *paper.ListDirFilesRequest) (bool, *model.UpYu
return false, nil, fmt.Errorf("service.GetDir: get dir info failed: %w", err)
}

err = s.cache.Paper.SetFileDirCache(s.ctx, key, *fileDir)
if err = s.cache.Paper.SetFileDirCache(s.ctx, key, *fileDir); err != nil {
return true, fileDir, fmt.Errorf("service.GetDir: set file dir cache failed: %w", err)
Copy link
Member Author

Choose a reason for hiding this comment

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

这个地方并不产生错误,他只是错误的传递者之一,于是使用fmt.Errorf()对其进行了错误链的封装

)

func (c *CachePaper) GetFileDirCache(ctx context.Context, key string) (bool, *model.UpYunFileDir, error) {
ret := &model.UpYunFileDir{}

data, err := c.client.Get(ctx, key).Bytes()
if err != nil {
return false, ret, fmt.Errorf("dal.GetFileDirCache: get dir info failed: %w", err)
return false, ret, errno.Errorf(errno.InternalDatabaseErrorCode, "dal.GetFileDirCache: get dir info failed: %v", err)
Copy link
Member Author

Choose a reason for hiding this comment

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

这里的错误是origin error,也就是产生错误的地方,所以我们使用errno中的函数来创建error,这样的话这个error会包含有当前的栈信息。

}
err = sonic.Unmarshal(data, &ret)
if err != nil {
return false, ret, fmt.Errorf("dal.GetFileDirCache: Unmarshal dir info failed: %w", err)
return false, ret, errno.Errorf(errno.InternalJSONErrorCode, "dal.GetFileDirCache: Unmarshal dir info failed: %v", err)
Copy link
Member Author

Choose a reason for hiding this comment

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

同上,产生错误的地方需要使用errno来包装一个栈信息

)

const TwoDay = 2 * constants.OneDay

func (c *CachePaper) SetFileDirCache(ctx context.Context, key string, dir model.UpYunFileDir) error {
data, err := sonic.Marshal(dir)
if err != nil {
return fmt.Errorf("dal.SetFileDirCache: Unmarshal dir info failed: %w", err)
return errno.Errorf(errno.InternalJSONErrorCode, "dal.SetFileDirCache: Unmarshal dir info failed: %v", err)
Copy link
Member Author

Choose a reason for hiding this comment

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

产生error的地方

}
return c.client.Set(ctx, key, data, TwoDay).Err()
if err = c.client.Set(ctx, key, data, TwoDay).Err(); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

产生

@SchwarzSail
Copy link
Member

对 api 模块的错误返回也可以写一下,我现在觉得 api 模块返回的错误和日志信息挺乱的,大致应该要分成给前端的(Format)和后端的( logger,调用栈)
还有,我觉得可以参考一下这篇文章,感觉写得挺好的:https://juejin.cn/post/7207078219216060474#heading-12

@mutezebra
Copy link
Member Author

对 api 模块的错误返回也可以写一下,我现在觉得 api 模块返回的错误和日志信息挺乱的,大致应该要分成给前端的(Format)和后端的( logger,调用栈) 还有,我觉得可以参考一下这篇文章,感觉写得挺好的:https://juejin.cn/post/7207078219216060474#heading-12

没问题,可以先把这个合并了。我开一个新pr来解决你说的

@SchwarzSail SchwarzSail enabled auto-merge (squash) December 3, 2024 08:27
@SchwarzSail
Copy link
Member

@jiuxia211 merge

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