-
Notifications
You must be signed in to change notification settings - Fork 435
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
[CONTINT-3554] Send the Datadog-Entity-ID
header, containing either the container-id or the cgroup inode if available
#2402
Conversation
…iner-ID is also sent without building the correct entityID yet
BenchmarksBenchmark execution time: 2023-12-14 16:54:03 Comparing candidate commit bd1d7cd in PR branch Found 5 performance improvements and 0 performance regressions! Performance is the same for 34 metrics, 2 unstable metrics. scenario:BenchmarkPartialFlushing/Disabled-24
scenario:BenchmarkPartialFlushing/Enabled-24
scenario:BenchmarkSingleSpanRetention/no-rules-24
scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24
scenario:BenchmarkSingleSpanRetention/with-rules/match-half-24
|
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.
Looks great, thanks! Let's get this merged once we have the changes in the agent!
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>
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 for data streams
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.
Thank you! One small request regarding the profiler unit tests. Otherwise LGTM.
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 for profiling, thanks!
92df5d9
to
58be3f0
Compare
Datadog-Entity-ID
header, containing either the container-id or the cgroupv2 inode if availableDatadog-Entity-ID
header, containing either the container-id or the cgroup inode if available
2df3dd0
to
4c2e2bb
Compare
4c2e2bb
to
e477776
Compare
internal/container.go
Outdated
if containerID != "" { | ||
return "cid-" + containerID | ||
} | ||
return getCgroupInode(mountsPath, cgroupPath) |
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 code is incorrect here, mountsPath
is /proc/mounts
, which is not read as we defaulted to always reading /sys/fs/cgroup
, it should be defaultCgroupMountPath
and mountsPath
should be removed.
Note that you have no unit test on that.
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.
Nice catch. I fixed it and added a unit test
Datadog-Entity-ID
header, containing either the container-id or the cgroup inode if availableDatadog-Entity-ID
header, containing either the container-id or the cgroup inode if available
@AliDatadog @vboulineau is this ready to merge? |
Waiting for the last approval |
internal/container.go
Outdated
) | ||
|
||
const ( | ||
// cgroupPath is the path to the cgroup file where we can find the container id if one exists. | ||
cgroupPath = "/proc/self/cgroup" | ||
|
||
// mountsPath is the path to the mounts file where we can find the cgroup v2 mount point. | ||
mountsPath = "/proc/mounts" |
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 used, should be removed
What does this PR do?
This PR sends the
Datadog-Entity-ID
header when theDatadog-Container-Id
is usually sent, containing either the container-id or the cgroup inode if available.Motivation
Using the cgroup inode will allow the trace-agent to retrieve the container-id, typically on cgroupv2 without UDS.
Drawbacks
Datadog-Entity-ID will be sent even if the app is not running in a container because there is no reliable way to ensure that the app is running in a container.
Test
This PR should be tested in several setups without UDS:
in-<inode>
with the inode obtain bykubectl exec <pod> -- stat /sys/fs/cgroup
kubectl exec <pod> -- stat /sys/fs/cgroup/memory
).With UDS, we should never use the inode logic.
Cgroupv1: Same as hybrid
To test e2e, use an image containing this PR [CONTINT-3554] Send the
Datadog-Entity-ID
header, containing either the container-id or the cgroup inode if available #2402 or directlydocker.io/alidatadog/agent:7-51-0-trace-containerid
on a kind 1.28 cluster using the following manifest:Example:
and build the image with the following
Dockerfile
To build a multi-arch image, I used
docker buildx build --push --platform=linux/amd64,linux/arm64 -t docker.io/alidatadog/dummy-apm-app:0.1.0 .
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!