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

Revert "Moving cgroup path name to field from tag to reduce cardinali… #1796

Closed
wants to merge 2 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Sep 21, 2016

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

…ty (#1457)"

This was introducing a regression with influxdb output, leading to
collision and points missing.
This reverts commit 53f4006.

Fixes #1724

@ghost ghost mentioned this pull request Sep 21, 2016
@sebito91
Copy link
Contributor

If you keep in mind that high cardinality can literally cause your influxdb to topple, and you're able to limit the unique pathnames you collect with then I'm sure this PR should be fine.

@nhaugo nhaugo added this to the 1.1.0 milestone Sep 21, 2016
@sparrc
Copy link
Contributor

sparrc commented Oct 4, 2016

@ririsoft for me I think it's OK to merge this because I think the cgroup plugin is substantially less useful without it.

That being said, can you add some documentation in SampleConfig and the README on limiting the cardinality that this plugin creates?

@sebito91 @daviesalex do you have any input on what patterns to avoid in the configuration of this plugin? This is the current README, and I'm not sure if the documented patterns along with using the paths as tags should be considered dangerous: https://github.com/influxdata/telegraf/tree/master/plugins/inputs/cgroup

Ririsoft added 2 commits October 6, 2016 21:44
…ty (#1457)"

This was introducing a regression with influxdb output, leading to
collision an points missing.
This reverts commit 53f4006.
This adds some documentation to the cgroup input plugin
to highlight potential influxdb cardinality issue with
a large number of cgroups.
@ghost
Copy link
Author

ghost commented Oct 6, 2016

@sparrc documentation and changelog updated as suggested. I took the opportunity to rebase against master. Cheers.

sparrc pushed a commit that referenced this pull request Oct 12, 2016
…ty (#1457)"

This was introducing a regression with influxdb output, leading to
collision an points missing.
This reverts commit 53f4006.

closes #1724
closes #1796
sparrc pushed a commit that referenced this pull request Oct 12, 2016
…ty (#1457)"

This was introducing a regression with influxdb output, leading to
collision an points missing.
This reverts commit 53f4006.

closes #1724
closes #1796
@sparrc sparrc closed this in #1888 Oct 12, 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.

3 participants