-
Notifications
You must be signed in to change notification settings - Fork 185
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
"container.id" resource attribute is wrong #1114
"container.id" resource attribute is wrong #1114
Comments
I think if it found a container id, it was by accident. Container detection is only supposed to work for docker containers: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.25.0/specification/resource/semantic_conventions/container.md and so it's probably a mistake that it's enabled in the demo at all. I can see that it's gotten the value from mountinfo, because it found a pattern that matches how docker does it. Looking at the output you've provided, it does look like we could work it out though. Is there any prior art for how other SIGs/APM implementations work it out? How reliable is My thoughts: the detector should be removed from the demo, or we should try to improve the detector so that it works in k8s. |
Hi @brettmc ,
There are 2 versions of cgroup using the 2 system files as I mentioned in previous post. For example, Java is doing well with following code which you can refer to: |
Our container detector was originally based on Java's implementation, so that makes it a bit easier. In this case, the "correct" container id according to their tests is from the line in mountinfo containing |
Hi @brettmc , |
I understand now. A few things have changed since I originally copied Java's implementation - we were checking V2 then V1, but that's since been reversed. I've got a PR in to fix this, do you know how to test it? |
@brettmc If there is no need to compile and build, I can test the code change in my environments. |
Hmm, yeah. If you could work out how to replace |
Hi @brettmc , where can I find the PR you mentioned to replace Container.php? I can test it in my K8S environment to see if it works. |
Hi @brettmc , Note: In the OTel demo of "https://opentelemetry.io/docs/demo/kubernetes-deployment/", the
In " https://github.com/brettmc/opentelemetry-php-contrib/blob/container-detector-k8s/src/ResourceDetectors/Container/src/Container.php", the namespace is:
So, I just used the namespace of the demo and copied all other code. |
That's great news! Yes, sorry, the container detector was moved out of the SDK to be spec-compliant, which is why the namespace was different. |
@liurui-1 this has been added to the demo app via open-telemetry/opentelemetry-demo#1114 |
Describe your environment Describe any aspect of your environment relevant to the problem, including your php version (
php -v
will tell you your current version), version numbers of installed dependencies, information about your cloud hosting provider, etc. If you're reporting a problem with a specific version of a library in this repo, please check whether the problem has been fixed on master.Steps to reproduce
We are using OpenTelemetry official demo in K8S:
https://opentelemetry.io/docs/demo/kubernetes-deployment/
The
quoteservice
is using php instrumentation.What is the expected behavior?
We want to see the "container.id" resource attribute of the pod for
quoteservice in my environment is
78ea929aa43e7b71f7c36583d82038d92a76800bf5da9b8850e8bd7b514bc075`This is reported by command: "kubectl describe pod my-otel-demo-quoteservice-xxxx" and other APM tools.
What is the actual behavior?
The "container.id" resource attribute reported by PHP OTel SDK is "c3e91a49d35e91f5ef2f672307a9ea830dbb80c488501ce2a6acb1cec7ee7b17"
Additional context
There are 2 system files which can be used to get container id from within a container. They are "/proc/self/cgroup" and "/proc/self/mountinfo". I think the PHP library may just get the ID from a wrong place.
Here is the data got from my env:
The text was updated successfully, but these errors were encountered: