Skip to content
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] Fix build on linux and do not send cgroup node inode if in host cgroup namespace #2453

Merged
merged 5 commits into from Dec 20, 2023

Conversation

AliDatadog
Copy link
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
@pr-commenter
Copy link

pr-commenter bot commented Dec 19, 2023

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%]

Copy link
Contributor

@nsrip-dd nsrip-dd left a comment

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?

@@ -3,6 +3,8 @@
// This product includes software developed at Datadog (https://www.datadoghq.com/).
// Copyright 2016 Datadog, Inc.

//go:build test && linux
Copy link
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.

Copy link
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

@nsrip-dd
Copy link
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
153 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants