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

#2527 Allow collecting of host stats within docker containers by enab… #2924

Merged

Conversation

MatthewCh
Copy link
Contributor

@MatthewCh MatthewCh commented Jun 14, 2017

Added fix for #2527

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@MatthewCh MatthewCh force-pushed the feature/override-proc-and-sys-dir branch 6 times, most recently from e2813b4 to ffbe55c Compare June 14, 2017 18:58
@danielnelson
Copy link
Contributor

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.

@MatthewCh
Copy link
Contributor Author

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.

@danielnelson
Copy link
Contributor

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 MatthewCh force-pushed the feature/override-proc-and-sys-dir branch 10 times, most recently from f57ff57 to 6591520 Compare June 21, 2017 18:57
@MatthewCh
Copy link
Contributor Author

MatthewCh commented Jun 21, 2017

Thanks @danielnelson for now I will have the change only for environment variables, and I have changed the names to match gopsutil.

@danielnelson
Copy link
Contributor

In order to match gopsutil, the HOST_SYS variable should point to the /sys location and HOST_PROC should point to the /proc location. All of the variables are documented here: https://github.com/shirou/gopsutil#usage

@MatthewCh MatthewCh force-pushed the feature/override-proc-and-sys-dir branch from 6591520 to f5aa49a Compare June 21, 2017 22:06
@MatthewCh
Copy link
Contributor Author

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 MatthewCh force-pushed the feature/override-proc-and-sys-dir branch from f5aa49a to 2657863 Compare June 21, 2017 22:08
@@ -23,6 +24,9 @@ func (_ SysctlFS) Description() string {
func (_ SysctlFS) SampleConfig() string {
return sysctlFSSampleConfig
}
func OSGetEnv(key string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

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",
Copy link
Contributor

Choose a reason for hiding this comment

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

Use path.Join for all of the paths

@MatthewCh MatthewCh force-pushed the feature/override-proc-and-sys-dir branch from 2657863 to b18c8d5 Compare June 22, 2017 16:42
@MatthewCh
Copy link
Contributor Author

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"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this should actually use GetHostProc too, since /proc/sys and /sys are not the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, looks like we are not actually using HOST_SYS here. I think the cgroups plugin is an example where it is needed.

@MatthewCh MatthewCh force-pushed the feature/override-proc-and-sys-dir branch 2 times, most recently from 3bdd53f to 4218e24 Compare June 22, 2017 21:17
@MatthewCh
Copy link
Contributor Author

Cool, I updated it, but I am unable to find where cgroup uses the /sys path.

@MatthewCh MatthewCh force-pushed the feature/override-proc-and-sys-dir branch from 4218e24 to 3a1657c Compare June 22, 2017 21:29
@MatthewCh MatthewCh force-pushed the feature/override-proc-and-sys-dir branch from 3a1657c to 5698b26 Compare June 22, 2017 21:39
@danielnelson danielnelson added this to the 1.4.0 milestone Jun 23, 2017
@danielnelson danielnelson merged commit 6d5bb35 into influxdata:master Jun 23, 2017
jeichorn pushed a commit to jeichorn/telegraf that referenced this pull request Jul 24, 2017
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