-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add docker with cgroup2 detection #116
Conversation
Add docker with cgroup2 test Refactor docker with cgroup1 detection Add docker with cgroup1 detection test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change makes sense.
end | ||
|
||
def in_docker_containr_with_cgroup2? | ||
File.exist?('/proc/1/mountinfo') && File.read('/proc/1/mountinfo') =~ %r{/docker/containers/} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This article suggests /proc/self/mountinfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing.
Fixed in the following commit.
845580b
@@ -85,7 +85,15 @@ def sent_from_docker_host?(request) | |||
end | |||
|
|||
def app_runs_in_docker_container? | |||
@app_runs_in_docker_container ||= `[ -f /proc/1/cgroup ] && cat /proc/1/cgroup` =~ /docker/ | |||
@app_runs_in_docker_container ||= in_docker_containr_with_cgroup1? || in_docker_containr_with_cgroup2? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way other than cgroups (i.e. dockerenv
file)? Based on my searching, it seems to be the best, but all options don't seem great.
I'm open to re-evaluating this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked for a way to detect if a process is running inside a docker container,
but there doesn't seem to be an official and portable way to detect it.
Maybe reading /proc/1/cgrops and /proc/self/mountinfo is the best workaround now.
References:
- Add auto-detection of container information to resource if cgroupv2 is used open-telemetry/opentelemetry-java-instrumentation#6694
- provide a portable mechanism for processes within container to obtain their image and container ids containerd/containerd#8185
- provide a portable mechanism for processes within container to obtain their image and container ids opencontainers/runtime-spec#1105
- https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/CgroupV2ContainerIdExtractor.java
- https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/CgroupV1ContainerIdExtractor.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the contribution!
@app_runs_in_docker_container ||= in_docker_containr_with_cgroup1? || in_docker_containr_with_cgroup2? | ||
end | ||
|
||
def in_docker_containr_with_cgroup1? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you noticed, but this should be container
(you forgot the e
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, it's typo.
thanks.
Fixed in the following commit.
21c62ec
Thanks for the PR! I'll merge it and will release a new version of the gem sometime today. |
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.