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

Pidstat plugin #3381

Closed
wants to merge 9 commits into from
Closed

Pidstat plugin #3381

wants to merge 9 commits into from

Conversation

PetrBarborka
Copy link

Required for all PRs:

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

@PetrBarborka
Copy link
Author

I have run go fmt ./... to format my code as circleci suggest. Why isn't it happy?

@danielnelson
Copy link
Contributor

Here is what I get with running go fmt:

(git::PetrBarborka-master) $ go fmt ./...
plugins/inputs/all/all.go
(git::PetrBarborka-master) $ git diff
diff --git a/plugins/inputs/all/all.go b/plugins/inputs/all/all.go
index 790d0133..05d81595 100644
--- a/plugins/inputs/all/all.go
+++ b/plugins/inputs/all/all.go
@@ -62,8 +62,8 @@ import (
        _ "github.com/influxdata/telegraf/plugins/inputs/openldap"
        _ "github.com/influxdata/telegraf/plugins/inputs/passenger"
        _ "github.com/influxdata/telegraf/plugins/inputs/phpfpm"
-       _ "github.com/influxdata/telegraf/plugins/inputs/ping"
        _ "github.com/influxdata/telegraf/plugins/inputs/pidstat"
+       _ "github.com/influxdata/telegraf/plugins/inputs/ping"
        _ "github.com/influxdata/telegraf/plugins/inputs/postgresql"
        _ "github.com/influxdata/telegraf/plugins/inputs/postgresql_extensible"
        _ "github.com/influxdata/telegraf/plugins/inputs/powerdns"

@danielnelson
Copy link
Contributor

Thank you for the pull request, pidstat looks like a nice utility, but I'm concerned about the amount of overlap between this and the current procstat plugin. We don't want to provide too many ways to do something because it can be confusing to the users.

Is there something that this plugin gathers that procstat doesn't, maybe we can address the shortcoming of procstat instead.

@PetrBarborka
Copy link
Author

PetrBarborka commented Oct 25, 2017

ad format: Thanks for your assistance, but I still don't understand the problem. It should be added into the "all.go", right? It is alphabetical order, right? Solved

ad relevancy: We need some things procstat won't give us:

  1. resource percentage
  2. full name of runnig command incl. arguments ( not there atm, comming right up )
  3. actual i/o data ( somehow procstat creates measurement fields for them but never sends any )

@danielnelson
Copy link
Contributor

What exactly do you mean by resource percentage? Usually we prefer to output the total and current and have percentage calculated on query, which has a few advantages when aggregating.

On the full command line, there has been some discussion on #1873, I think concerns about potential command length it would apply here too. However, I have no other good ideas so perhaps we just require the user to opt-in. I'll comment more on the issue.

On the IO metrics, these require root permissions to gather. This is problematic because we don't recommend running telegraf as root. There may be a way to change this but I have not been able to figure out how #2787

@danielnelson
Copy link
Contributor

Closing pull request, I think we will want to focus on improving the procstat input instead of adding support for shelling out to external utilities.

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