-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Output HostStatsCollector's Collect errors #1535
Output HostStatsCollector's Collect errors #1535
Conversation
@@ -80,6 +83,8 @@ func (h *HostStatsCollector) Collect() (*HostStats, error) { | |||
Free: memStats.Free, | |||
} | |||
hs.Memory = ms | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the if/else
statements.
memStats, err := mem.VirtualMemory()
....
if err != nil {
return nil, err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diptanu I agree that removing if/else
statements. But return nil, err
is OK?
if err != nil {
return nil, err
}
I thought it should collect errors as below like you commented.
mErr.Errors = append(mErr.Errors, fmt.Errorf("error to collect cpu stats", err))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nak3 Yeah this is ok, since even if we collect more stats but return an error we won't update the stats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diptanu I see. Thank you.
I initially didn't want to report any errors since I wanted to collect as much stats I could in the event stats collection of a particular sub-system was failing. |
@@ -139,7 +139,7 @@ func TestConfig_Merge(t *testing.T) { | |||
"foo": "bar", | |||
"baz": "zip", | |||
}, | |||
ChrootEnv: map[string]string{}, | |||
ChrootEnv: map[string]string{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a fix for gofmt error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@diptanu Thank you. I updated. |
if err != nil { | ||
return nil, err | ||
} | ||
ms := &MemoryStats{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hs.Memory := &MemoryStats {
@diptanu Thank you. I updated. |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
No description provided.