-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add container id support to Resource #2418
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2418 +/- ##
=====================================
Coverage 75.7% 75.8%
=====================================
Files 172 173 +1
Lines 11619 11664 +45
=====================================
+ Hits 8800 8842 +42
- Misses 2609 2612 +3
Partials 210 210
|
I was checking this PR, I was wondering if the approach I took in https://github.com/hypertrace/goagent/blob/main/sdk/internal/container/container.go was simpler or am I missing something. I also wonder, do we want an integration test like https://github.com/hypertrace/goagent/tree/main/tests/docker. I could port it here @XSAM |
This test case shows a cgroup line may contain the prefix and the suffix. opentelemetry-go/sdk/resource/container_test.go Lines 49 to 51 in bf3c93c
Also, here is an example of the cgroup string that is fetched from my Kubernetes cluster. You can see the cgroup string becomes complicated.
Good point. I also wonder about this. To achieve this integration test, we need container runtime. It means to run the tests, it requires all contributors to install a container runtime. It also increases the dependence on this project. Not sure it is worth doing that. Any thoughts? @open-telemetry/go-approvers |
BTW, I found the approach of this PR is not working for cgroup v2. In cgroup v2, we only get little info from the cgroup file. For instance,
|
I am curious about this. Instead of opening up an API to this I would focus
all efforts to get a unique strong implementation. Are you targeting a
concrete case?
…On Thu, Dec 9, 2021, 22:13 Steven E. Harris ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In sdk/resource/container.go
<#2418 (comment)>
:
> + defaultContainerIDProvider containerIDProvider = getContainerIDFromCGroup
+)
+
+type containerIDDetector struct{}
+
+const cgroupPath = "/proc/self/cgroup"
Given that this detector uses a specific technique to find this
container's ID—this one assuming that one extraced from the cgroup file is
the right choice—would you be amenable to naming this detector more
specifically? Perhaps cgroupContainerIDDetector? That would clarify where
this ID is coming from, and open us to other approaches to finding a
container's ID.
I'm not sure whether it would warrant a corresponding change to the
proposed WithContainerID function.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2418 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYASTLK2CLGHPJMAVBBLUQELY5ANCNFSM5IZYPSYA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Are you asking me? If so, I'm telegraphing the conversation from today's Go SIG meeting—a conversation that I started. There will be other interpretation of what "a container's ID" is or what it means. I don't know what they are. I know enough to doubt the universality of this particular choice. |
So I approved this with my understanding at the time that both cgroups were made in a response to containers, and that the current Linux runtimes, that I know of, all use cgroups under the hood to manage containers. While both the OCI, and the CRI require a container ID, neither specifies that you must use cgroups to do so. I don't know if there is an implementation counter to this, but I will admit I haven't checked all of them. With that in mind, these would be my recommendations:
|
FYI, I created a comment on OCI spec looking for an official/stable way to fetch container id within the container. |
This should be a long-term direction and not block this PR. |
I believe this PR is ready for review. |
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!
Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>
* Add container id support to Resource * Fix wrong test case name * Add WithContainer option * Update CHANGELOG * Fix comments * Update CHANGELOG * Use regex to find container id * Add tests for reading cgroup file * Update sdk/resource/container.go Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com> * Update format Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com> Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
resolves #1981
Prior Art
cc: @pavankrish123