Skip to content

Update procstat to support cgroup globs & include systemd unit children#7890

Closed
DEvil0000 wants to merge 7 commits intoinfluxdata:masterfrom
wingcon:improve_procstat
Closed

Update procstat to support cgroup globs & include systemd unit children#7890
DEvil0000 wants to merge 7 commits intoinfluxdata:masterfrom
wingcon:improve_procstat

Conversation

@DEvil0000
Copy link
Copy Markdown
Contributor

  • cgroups now support glob
  • systemd_unit in combination with systemd_all will now report for all children and not just the main pid of the service
  • in case cgroups or systemd_unit+all will return a group of pids they get tagged with their actual cgroup/service matched (cgroup_full)
  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@TelmoM
Copy link
Copy Markdown

TelmoM commented Jan 15, 2021

Hi @sjwang90, is this update going to be merged soon?
Thank you

@sjwang90
Copy link
Copy Markdown
Contributor

Sorry for the delay - we'll get this reviewed.

Copy link
Copy Markdown
Contributor

@sspaink sspaink left a comment

Choose a reason for hiding this comment

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

Thank you for working on this pull request! It does look it will improve the procstat plugin, I've added some review comments for you to look at.

## user as argument for pgrep (ie, pgrep -u <user>)
# user = "nginx"
## Systemd unit name
## Systemd unit name or glob and if all processes of the unit should get collected
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't the "or glob..." line also be above the cgroup setting? And it seems systemd_unit only supports globs when systemd_all is set to true.

## Systemd unit name
## Systemd unit name or glob and if all processes of the unit should get collected
# systemd_unit = "nginx.service"
# systemd_all = true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe this should be renamed to something like include_systemd_unit_children, that would be more descriptive. Thoughts?

# user = "nginx"
## Systemd unit name
# systemd_unit = "nginx.service"
# systemd_all = true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be false to not change original behavior

@sspaink sspaink changed the title Improve procstat Update procstat to support cgroup globs & include systemd unit children Jul 6, 2021
@sspaink sspaink added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jul 6, 2021
@DEvil0000
Copy link
Copy Markdown
Contributor Author

@sspaink thanks for the review!
However go is not one of the programming languages I use (at all) so I tried to do it in a kind of clean way and sticking close to the original code. About 1y after creating the PR I don't feel like I want to mess with those details (at least currently I don't have time to do so - sorry). Please fix or change what you feel meets the projects coding style or standards best e.g. in a additional commit.
I would absolutely appreciate having this features in the master so I may be able to go back to the official version instead of maintaining my own one.

@sspaink
Copy link
Copy Markdown
Contributor

sspaink commented Jul 6, 2021

@DEvil0000 thank you for letting us know, sorry for taking so long with the review. I'll look into taking over the pull request and work with the team to get it merged. I will be sure mention your contribution and let you know once it is in master/released.

@DEvil0000
Copy link
Copy Markdown
Contributor Author

Thank you! Thank you for taking care!

@sspaink
Copy link
Copy Markdown
Contributor

sspaink commented Jul 10, 2021

#9488 will continue this pull request.

@sspaink sspaink closed this Jul 10, 2021
sspaink added a commit that referenced this pull request Jul 27, 2021
phemmer added a commit to phemmer/telegraf that referenced this pull request Aug 13, 2021
* origin/master: (183 commits)
  fix: CrateDB replace dots in tag keys with underscores (influxdata#9566)
  feat: Pull metrics from multiple AWS CloudWatch namespaces (influxdata#9386)
  fix: improve Clickhouse corner cases for empty recordset in aggregation queries, fix dictionaries behavior (influxdata#9401)
  fix(opcua): clean client on disconnect so that connect works cleanly (influxdata#9583)
  fix: Refactor ec2 init for config-api (influxdata#9576)
  fix: sort logs by timestamp before writing to Loki (influxdata#9571)
  fix: muting tests for udp_listener (influxdata#9578)
  fix: Do not return on disconnect to avoid breaking reconnect (influxdata#9524)
  fix: Fixing k8s nodes and pods parsing error (influxdata#9581)
  feat: OpenTelemetry output plugin (influxdata#9228)
  feat: Support AWS Web Identity Provider (influxdata#9411)
  fix: upgraded sensu/go to v2.9.0 (influxdata#9577)
  fix: Normalize unix socket path (influxdata#9554)
  docs: fix aws ec2 readme inconsistency (influxdata#9567)
  feat: Modbus Rtu over tcp enhancement (influxdata#9570)
  docs: information on new conventional commit format (influxdata#9573)
  docs: Add logo (influxdata#9574)
  docs: Adding links to net_irtt and dht_sensor external plugins (influxdata#9569)
  Upgrade hashicorp/consul/api to 1.9.1 (influxdata#9565)
  Update vmware/govmomi to v0.26.0 (influxdata#9552)
  Do not skip good quality nodes after a bad quality node is encountered (influxdata#9550)
  fix test so it hits a fake service (influxdata#9564)
  Update changelog
  Fix procstat plugin README to match sample config (influxdata#9553)
  Fix metrics reported as written but not actually written  (influxdata#9526)
  Prevent segfault in persistent volume claims (influxdata#9549)
  Update procstat to support cgroup globs & include systemd unit children (Copy of influxdata#7890) (influxdata#9488)
  Fix attempt to connect to an empty list of servers. (influxdata#9503)
  Fix handling bool in sql input plugin (influxdata#9540)
  Suricata alerts (influxdata#9322)
  Linter fixes for plugins/inputs/[fg]* (influxdata#9387)
  For Prometheus Input add ability to query Consul Service catalog (influxdata#5464)
  Support Landing page on Prometheus landing page (influxdata#8641)
  [Docs] Clarify tagging behavior (influxdata#9461)
  Change the timeout from all queries to per query (influxdata#9471)
  Attach the pod labels to the `kubernetes_pod_volume` & `kubernetes_pod_network` metrics. (influxdata#9438)
  feat(http_listener_v2): allows multiple paths and add path_tag (influxdata#9529)
  Bug Fix Snmp empty metric name (influxdata#9519)
  Worktable workfile stats (influxdata#8587)
  Update Go to v1.16.6 (influxdata#9542)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/procstat feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants