Skip to content

Add cgroup support (#831) #846

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Nov 18, 2020
Merged

Add cgroup support (#831) #846

merged 12 commits into from
Nov 18, 2020

Conversation

tanquetav
Copy link
Contributor

@tanquetav tanquetav commented Jun 2, 2020 •
edited by basepi
Loading

What does this PR do?

closes #831
closes #876

Checklist

  • My code follows the style guidelines of this project
  • I have rebased my changes on top of the latest master branch
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated CHANGELOG.asciidoc
  • I have updated supported-technologies.asciidoc
  • Added an API method or config option? Document in which version this will be introduced
  • Added an instrumentation plugin? How did you make sure that old, non-supported versions are not instrumented by accident?

Author's Checklist

Related issues

Use cases

When a java program is running on a cgroup limited environment (docker with -m option, k8s with resource limit) cgroup is used to get memory information(system.memory.total, system.memory.actual.free and system.process.memory.rss)

Screenshots

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 2, 2020 •
edited
Loading

💚 CLA has been signed

@apmmachine
Copy link
Contributor

apmmachine commented Jun 2, 2020 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #846 updated]

  • Start Time: 2020-11-18T07:53:52.343+0000

  • Duration: 22 min 21 sec

Test stats 🧪

Test Results
Failed 0
Passed 12135
Skipped 8375
Total 20510

Steps errors 1

Expand to view the steps failures

Shell Script

  • Took 1 min 28 sec . View more details on here
  • Description: ./tests/scripts/docker/run_tests.sh python-2.7 none

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 12135
Skipped 8375
Total 20510

houndci-bot
houndci-bot reviewed Jun 5, 2020
View reviewed changes
@tanquetav tanquetav mentioned this pull request Jul 11, 2020
Closed
@beniwohli beniwohli self-assigned this Oct 28, 2020
@beniwohli
Copy link
Contributor

@tanquetav sorry for letting this linger for so long! I took the liberty to change some variable/method names so they are more aligned with our code style, as well as adding some comments and a few more tests.

I think we can merge this now, what do you think @basepi?

basepi
basepi approved these changes Oct 30, 2020
View reviewed changes
Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

Sounds good to me!

@apmmachine
Copy link
Contributor

apmmachine commented Oct 30, 2020 •
edited
Loading

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 11439
Skipped 8409
Total 19848

@tanquetav
Copy link
Contributor Author

Let me check it out this weekend. I made some changes on java agent and let me assure the both logic are equals.

@basepi
Copy link
Contributor

basepi commented Nov 4, 2020

@tanquetav Did you get a chance to review this again?

@drnextgis
Copy link

Looking forward to have it being merged 🙏

@beniwohli
Copy link
Contributor

I compared the code with the analogue code in the Java agent, and they seem to be functionally identical. I'll go ahead and merge this.

@beniwohli beniwohli merged commit 82e4c62 into elastic:master Nov 18, 2020
@drnextgis
Copy link

The title of the PR is a little bit misleading. Please correct me if I'm wrong but as far as I understood this PR enables memory cgroup-based metrics (which is great!) but we still lack cgroup-based CPU metrics? If so I think it worth to file a separate issue to track the status.

@beniwohli
Copy link
Contributor

@drnextgis yes, the PR title doesn't reflect that this just about memory. In the changelog of 5.10.0, we noted that it's only about memory.

Adding CPU metrics is planned, you can follow along here: elastic/apm#368

beniwohli added a commit to beniwohli/apm-agent-python that referenced this pull request Sep 14, 2021
@tanquetav @beniwohli @basepi
Add cgroup support (elastic#831) (elastic#846)
b7cee53 
* Add cgroup support (elastic#831)

* Fix search to cgroup directory, looking for where it is mounted

* Fix None

* Change metrics names

* Round fix

* adapted code style to pep8, added some comments and a couple more tests

* use tighter exception handling, and removed a couple of unnecessary `return None`

Co-authored-by: Benjamin Wohlwend <beni@elastic.co>
Co-authored-by: Colton Myers <colton.myers@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@basepi basepi basepi approved these changes

Assignees

@beniwohli beniwohli

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reporting and showing cgroup-based metrics APM Agent on K8s take system memory from host
6 participants
@tanquetav @apmmachine @beniwohli @basepi @drnextgis @houndci-bot