Fix self metrix when containerised #6620 #6641
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
| @@ -59,6 +65,10 @@ type Stats struct { | |||
| envRegexps []match.Matcher // List of regular expressions used to whitelist env vars. | |||
| } | |||
|
|
|||
| type LocalStats struct { | |||
There was a problem hiding this comment.
exported type LocalStats should have comment or be unexported
| @@ -19,6 +19,12 @@ import ( | |||
| sigar "github.com/elastic/gosigar" | |||
| ) | |||
|
|
|||
| var InitSigarProcd string | |||
There was a problem hiding this comment.
exported var InitSigarProcd should have comment or be unexported
|
|
||
| func (procStats *LocalStats) preGetOne() { | ||
| curSigarProcd = sigar.Procd | ||
| sigar.Procd = InitSigarProcd |
There was a problem hiding this comment.
Solving it this way would probably require a locking mechanism in gosigar, as this value is global and there can be other goroutines using it at the same time.
There was a problem hiding this comment.
If this PR will be accepted in general, I think it can be done without big changes of gosigar.
|
|
||
| import "os" | ||
|
|
||
| func GetSelfPid() (int, error) { |
There was a problem hiding this comment.
exported function GetSelfPid should have comment or be unexported
| "strings" | ||
| ) | ||
|
|
||
| func GetSelfPid() (int, error) { |
There was a problem hiding this comment.
exported function GetSelfPid should have comment or be unexported
| "github.com/elastic/gosigar" | ||
| ) | ||
|
|
||
| func GetSelfPid() (int, error) { |
There was a problem hiding this comment.
exported function GetSelfPid should have comment or be unexported
|
|
||
| // GetSelfPid return self pid for current running process | ||
| func GetSelfPid() (int, error) { | ||
| contents, err := ioutil.ReadFile(gosigar.Procd + "/self/stat") |
There was a problem hiding this comment.
I was just thinking that it could be enough with os.Readlink(gosigar.Procd + "/self"), then no more parsing would be needed. Could you try? Thanks!
|
|
||
| // GetSelfPid return self pid for current running process | ||
| func GetSelfPid() (int, error) { | ||
| contents, err := ioutil.ReadFile(gosigar.Procd + "/self/stat") |
There was a problem hiding this comment.
Btw, to join paths it'd be better to use path.Join.
| @@ -0,0 +1,28 @@ | |||
| package process | |||
There was a problem hiding this comment.
Oh, this won't be needed as the file has the _linux suffix.
| @@ -0,0 +1,10 @@ | |||
| // +build darwin freebsd windows | |||
|
@jsoriano thanks for review. Agree with everything and already made changes. ReadLink works. Check please. Only requested "// +build linux" for _linux file make no sense, as you already mention. |
libbeat/cmd/instance/metrics.go
Outdated
| state, err := beatProcessStats.GetOne(beatPID) | ||
| pid, err := process.GetSelfPid() | ||
| if err != nil { | ||
| return 0, fmt.Errorf("error retrieving process stats: %v", err) |
There was a problem hiding this comment.
Rewrite to more specific message?
libbeat/cmd/instance/metrics.go
Outdated
| state, err := beatProcessStats.GetOne(beatPID) | ||
| pid, err := process.GetSelfPid() | ||
| if err != nil { | ||
| return 0.0, nil, fmt.Errorf("error retrieving process stats: %v", err) |
| import ( | ||
| "os" | ||
| "strconv" | ||
|
|
There was a problem hiding this comment.
This newline could be dropped to group all stdlib imports together.
| "github.com/elastic/gosigar" | ||
| ) | ||
|
|
||
| // GetSelfPid return self pid for current running process |
There was a problem hiding this comment.
How about "GetSelfPid returns the PID for this process."
|
jenkins, test this please |
|
@jsoriano I squash it and change error to more specific, but a little bit surprised, since github allow to do that. Or just to simplify commit message? @andrewkroh done group of stdlib imports and doc for function (but I did it without dot at the end, since docs everywhere without dots at the end). Thanks for review, so small diff at the end, but so much effort. |
|
@ewgRa thanks for taking care of this! Yes, I asked you to squash it to have a simpler message, and also a message that you chose as author of the changes :) |
|
Failing test doesn't seem related, merging this. |
(cherry picked from commit 369dd35)
(cherry picked from commit 369dd35)
(cherry picked from commit 369dd35)
This is a fix for #6620.
Probably it is a little bit dirty solution, but on other hand there is big refactoring of gosigar and there is a big question about later upstream merging.