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:#96 #98

Merged
merged 5 commits into from
May 19, 2022
Merged

feat:#96 #98

merged 5 commits into from
May 19, 2022

Conversation

songzhibin97
Copy link
Member

No description provided.

holmes.go Outdated
@@ -72,7 +72,7 @@ type Holmes struct {
}

type ProfileReporter interface {
Report(pType string, buf []byte, reason string, eventID string) error
Report(pType string, dumpName string, reason string, eventID string) error
Copy link
Member

Choose a reason for hiding this comment

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

fileName could be better.

writer.WriteField("token", r.token) // nolint: errcheck
writer.WriteField("profile_type", ptype) // nolint: errcheck
writer.WriteField("dumpName", dumpName) // nolint: errcheck
Copy link
Member

Choose a reason for hiding this comment

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

这个 reporter,需要直接把文件内容也传上去的呢
此处应该是需要读文件呢,这里的服务端接口要求的呢

Copy link
Member Author

Choose a reason for hiding this comment

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

入参是filename,在这里读取文件是吧

Copy link
Member

Choose a reason for hiding this comment

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

嗯,对的~

Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

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

其他没啥问题了~

writer.WriteField("token", r.token) // nolint: errcheck
writer.WriteField("profile_type", ptype) // nolint: errcheck
writer.WriteField("filename", filename) // nolint: errcheck
Copy link
Member

Choose a reason for hiding this comment

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

这里可以删掉了呢,不是需要的字段~

return fmt.Errorf("write buf to file part err: %v", err)

// read filename
if filename != "" {
Copy link
Member

Choose a reason for hiding this comment

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

这个判断感觉可以删掉,caller 会保证 filename 不是空

如果这里判断的话,为空的时候,就应该报错,而不是继续上报呢

Copy link
Member Author

Choose a reason for hiding this comment

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

有个测试过不了...

Copy link
Member Author

Choose a reason for hiding this comment

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

需要加个测试的空文件?

@doujiang24 doujiang24 merged commit 5b19423 into mosn:master May 19, 2022
@doujiang24
Copy link
Member

@songzhibin97 Thanks!

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

Successfully merging this pull request may close these issues.

2 participants