Skip to content

[CONTINT-3554] Fix build on linux and do not send cgroup node inode if in host cgroup namespace#2453

Merged
nsrip-dd merged 5 commits into
mainfrom
ali/fix-cid-retrieval-on-host
Dec 20, 2023
Merged

[CONTINT-3554] Fix build on linux and do not send cgroup node inode if in host cgroup namespace#2453
nsrip-dd merged 5 commits into
mainfrom
ali/fix-cid-retrieval-on-host

Conversation

@AliDatadog

@AliDatadog AliDatadog commented Dec 19, 2023

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR adds build flags in the logic that retrieves the container id since it only works on linux.
It also only retrieves the cgroup node inode if the sender not in the host cgroup namespace. Indeed it would mean either that:

  • We're not in a container and the inode we're sending is not related to a container
  • We already have access to the container-id and it will be retrieved in the previous logic

Motivation

Fix build and do not emit data if not necessary (limit perf impact)

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.

For Datadog employees:

  • If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@AliDatadog
AliDatadog requested a review from a team as a code owner December 19, 2023 17:32
@AliDatadog AliDatadog changed the title Fix build on linux and do not send cgroup node inode if not in host cgroup namespace [CONTINT-3554] Fix build on linux and do not send cgroup node inode if not in host cgroup namespace Dec 19, 2023
@nsrip-dd nsrip-dd mentioned this pull request Dec 19, 2023
Closed
7 tasks
@pr-commenter

pr-commenter Bot commented Dec 19, 2023 •
edited
Loading

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2023-12-19 18:40:33

Comparing candidate commit d27c2f9 in PR branch ali/fix-cid-retrieval-on-host with baseline commit 4ae528b in branch main.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 38 metrics, 2 unstable metrics.

scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24

  • 🟥 execution_time [+5.740µs; +7.442µs] or [+2.302%; +2.984%]

nsrip-dd
nsrip-dd requested changes Dec 19, 2023
View reviewed changes

@nsrip-dd nsrip-dd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! One change needed for the test file build tags (see my comment). Should someone with cgroups knowledge review the other changes?

Comment thread internal/container_linux_test.go Outdated
// This product includes software developed at Datadog (https://www.datadoghq.com/).
// Copyright 2016 Datadog, Inc.

//go:build test && linux

@nsrip-dd nsrip-dd Dec 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The test build tag isn't defined, so these tests won't run. I think you just want the linux tag.

@AliDatadog AliDatadog Dec 19, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I just removed it !

I also asked a review to my team members. They should have the knowledge necessary. Normally I am just aligning on what is done in dogstatsd DataDog/datadog-go#291

@AliDatadog
AliDatadog requested a review from nsrip-dd December 19, 2023 18:15
@nsrip-dd

nsrip-dd commented Dec 20, 2023

Copy link
Copy Markdown
Contributor

Changes look similar to the linked datadog-go changes, which were approved by someone with more knowledge, so LGTM

@nsrip-dd
nsrip-dd enabled auto-merge (squash) December 20, 2023 15:31
@AliDatadog AliDatadog changed the title [CONTINT-3554] Fix build on linux and do not send cgroup node inode if not in host cgroup namespace [CONTINT-3554] Fix build on linux and do not send cgroup node inode if in host cgroup namespace Dec 20, 2023
auto-merge was automatically disabled December 20, 2023 15:42

Pull Request is not mergeable

@nsrip-dd
nsrip-dd merged commit 3dfd935 into main Dec 20, 2023
@nsrip-dd
nsrip-dd deleted the ali/fix-cid-retrieval-on-host branch December 20, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nsrip-dd nsrip-dd nsrip-dd approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@AliDatadog @nsrip-dd