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

Add sysstat plugin #939

Closed
wants to merge 13 commits into from
Closed

Add sysstat plugin #939

wants to merge 13 commits into from

Conversation

zbindenren
Copy link
Contributor

Add sysstat plugin.

@zbindenren
Copy link
Contributor Author

circleci failed because of:

fatal: unable to access 'https://gopkg.in/fatih/pool.v2/': The requested URL returned error: 502

This is probably not because of my plugin.

@sparrc
Copy link
Contributor

sparrc commented Mar 30, 2016

I'll kick a rebuild

@sparrc
Copy link
Contributor

sparrc commented Mar 30, 2016

thanks for the contribution @zbindenren, but I think there might be a better way to handle the collection interval.

You could, for instance, cache the timestamp of the previous call to Gather(). In this way, you wouldn't be able to gather metrics on the first collection (similar to the cpu usage plugin), but you also wouldn't need users to configure their collection interval into the plugin.

@zbindenren
Copy link
Contributor Author

@sparrc you are right, I don't like the interval definition either. At first I was looking for something like:

acc.Interval() time.Duration

But then I realized, that this won't get implemented (#478).

I will try your suggestion.

@zbindenren
Copy link
Contributor Author

@sparrc I made the requested changes. Let me know what you think.

## and represents itself a measurement.
##
## If Group is true, corresponding metrics are grouped to a single measurement.
# group = false
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer group=true by default, because this is more consistent with the rest of telegraf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done with 7256f3e

@sparrc
Copy link
Contributor

sparrc commented Apr 6, 2016

@zbindenren I'm getting this error running on ubuntu 14.04 & centos 6.7:

Error in input [sysstat]: command /usr/bin/sadf -p -- -p -r ALL /tmp/sysstat-1459982740 failed with exit status 1

configuration is default, except group=true and sadc_path="/usr/lib/sysstat/sadc" & sadc_path="/usr/lib64/sa/sadc"

## Activities is a list of activities, that are passed as argument to the
## sadc collector utility (e.g: DISK, SNMP etc...)
## The more activities that are added, the more data is collected.
# activities = ["DISK"]
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be uncommented by default? since the option below is uncommented by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sadc_path is commented because I did not define a default value in the code. I can set the Ubuntu default value if you want, then I can comment out the value.

Group: true,
Activities: dfltActivities,
}
sadf, _ := exec.LookPath("sadf")
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you do this? I think this will look at the sadf path in the build system, but we want to look at the path on the runtime system nevermind, I'm wrong

@zbindenren
Copy link
Contributor Author

@sparrc I made the changes. The error you got, was because I used an sadf option (-r ALL), that is only available on newer linux distributions. I changed the option to -r in sample-config and Readme.md. This option also works on older distributions.

@sparrc sparrc closed this in b534b58 Apr 7, 2016
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