#2527 Allow collecting of host stats within docker containers by enab… #2924
Conversation
MatthewCh
force-pushed
the
feature/override-proc-and-sys-dir
branch
6 times, most recently
from
e2813b4 to
ffbe55c
Compare
|
If we are going to use an environment variable we should use HOST_PROC and HOST_SYS to match gopsutil, though I'd prefer to use options on the plugins. |
|
Agreed. But I wasn't sure if these were the only 2 plugins that could take these overrides, if not it would be easier to use environment variables so we wouldn't have to redefine the same options on multiple plugins. But I will make the proposed change since it doesn't seem like that is the case. |
|
It is a good point about wanting to share these, it could be good to have support for either or even both methods. If you do the environment variables though lets just use the same names as gopsutil since other users are already setting it for other plugins #1544 (comment) |
MatthewCh
force-pushed
the
feature/override-proc-and-sys-dir
branch
10 times, most recently
from
f57ff57 to
6591520
Compare
|
Thanks @danielnelson for now I will have the change only for environment variables, and I have changed the names to match gopsutil. |
|
In order to match gopsutil, the |
MatthewCh
force-pushed
the
feature/override-proc-and-sys-dir
branch
from
6591520 to
f5aa49a
Compare
|
Thanks, understood. I have made the change and I will retest it tomorrow. If you could take a quick look that would be helpful, i'm still relatively new to go. |
MatthewCh
force-pushed
the
feature/override-proc-and-sys-dir
branch
from
f5aa49a to
2657863
Compare
| @@ -23,6 +24,9 @@ func (_ SysctlFS) Description() string { | |||
| func (_ SysctlFS) SampleConfig() string { | |||
| return sysctlFSSampleConfig | |||
| } | |||
| func OSGetEnv(key string) string { | |||
There was a problem hiding this comment.
These functions don't really encapsulate anything, you could remove them or maybe you could have them return the default value when unset and call it something like GetHostProc() string or GetHostSys() string.
| inputs.Add("linux_sysctl_fs", func() telegraf.Input { | ||
| return &SysctlFS{ | ||
| path: "/proc/sys/fs", | ||
| path: sysPath + "/fs", |
There was a problem hiding this comment.
Use path.Join for all of the paths
MatthewCh
force-pushed
the
feature/override-proc-and-sys-dir
branch
from
2657863 to
b18c8d5
Compare
|
Thanks that helped. I have made the appropriate changes. |
| inputs.Add("linux_sysctl_fs", func() telegraf.Input { | ||
| return &SysctlFS{ | ||
| path: "/proc/sys/fs", | ||
| path: path.Join(GetHostSys(), "/fs"), |
There was a problem hiding this comment.
I guess this should actually use GetHostProc too, since /proc/sys and /sys are not the same.
There was a problem hiding this comment.
Are you saying it should be more like path.Join(GetHostProc(), "/sys/fs")?
I am not sure would HOST_SYS be of any use then?
There was a problem hiding this comment.
Yes, looks like we are not actually using HOST_SYS here. I think the cgroups plugin is an example where it is needed.
MatthewCh
force-pushed
the
feature/override-proc-and-sys-dir
branch
2 times, most recently
from
3bdd53f to
4218e24
Compare
|
Cool, I updated it, but I am unable to find where cgroup uses the /sys path. |
MatthewCh
force-pushed
the
feature/override-proc-and-sys-dir
branch
from
4218e24 to
3a1657c
Compare
…e procfs and sysfs location overrides
MatthewCh
force-pushed
the
feature/override-proc-and-sys-dir
branch
from
3a1657c to
5698b26
Compare
Added fix for #2527
Required for all PRs: