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

Cleanup cgroup_summary collector #2414

Merged
merged 1 commit into from
Jun 24, 2022
Merged

Cleanup cgroup_summary collector #2414

merged 1 commit into from
Jun 24, 2022

Conversation

SuperQ
Copy link
Member

@SuperQ SuperQ commented Jun 24, 2022

  • Correctly name collector file.
  • Fix cgroup summary type as gauge.
  • Use a boolean metric rather than a label for enabled.

Signed-off-by: Ben Kochie [email protected]

@SuperQ SuperQ requested review from discordianfish and Nexucis June 24, 2022 10:17
@SuperQ SuperQ force-pushed the superq/cgroup_cleanup branch 2 times, most recently from 905dbbb to a42dace Compare June 24, 2022 10:30
@Nexucis
Copy link
Member

Nexucis commented Jun 24, 2022

@SuperQ looks like the script is not happy with the name of the metric :)

@SuperQ
Copy link
Member Author

SuperQ commented Jun 24, 2022

Hmm, that's an annoying linter error. I guess we could just call this collector cgroups, since it reads from /proc/cgroups.

* Correctly name collector file.
* Fix cgroup summary type as gauge.
* Use a boolean metric rather than a label for enabled.

Signed-off-by: Ben Kochie <[email protected]>
@SuperQ SuperQ force-pushed the superq/cgroup_cleanup branch from a42dace to 1de82b3 Compare June 24, 2022 12:35
@SuperQ
Copy link
Member Author

SuperQ commented Jun 24, 2022

Ok, all fixed up.

@Nexucis
Copy link
Member

Nexucis commented Jun 24, 2022

mmm that means there is a breaking change in the collector name and in its behaviour ( it's a counter anymore but a gauge).
I'm wondering how much is it ok to do this since the node exporter is released under the v1 version.

I'm not super familiar with the release political on this project, but the code looks ok.
So if you think it's ok to merge @SuperQ, fine for me as well.

@SuperQ
Copy link
Member Author

SuperQ commented Jun 24, 2022

This change was just merged today, and has not been released, so not breaking.

@SuperQ SuperQ merged commit a516d4d into master Jun 24, 2022
@SuperQ SuperQ deleted the superq/cgroup_cleanup branch June 24, 2022 15:15
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
* Correctly name collector file.
* Fix cgroup summary type as gauge.
* Use a boolean metric rather than a label for enabled.

Signed-off-by: Ben Kochie <[email protected]>
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
* Correctly name collector file.
* Fix cgroup summary type as gauge.
* Use a boolean metric rather than a label for enabled.

Signed-off-by: Ben Kochie <[email protected]>
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