-
Notifications
You must be signed in to change notification settings - Fork 434
tracer: translate PIDs from descendant namespaces #1801
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
Open
simonepri
wants to merge
6
commits into
open-telemetry:main
Choose a base branch
from
simonepri:fix/descendant-pid-namespace-translation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
19a5a50
tracer: support descendant PID namespaces
simonepri e7ea864
tracer: simplify descendant PID namespace setup
simonepri e8c1434
tracer: expose descendant PID namespace mode
simonepri 1afef64
tracer: select PID translation mode automatically
simonepri f12f5e3
tracer: keep PID namespace test target alive
simonepri 7ee6010
tracer: rebuild eBPF blobs
simonepri File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
Binary file not shown.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Just sharing my initial impressions:
It looks like BTF becomes a hard requirement with this change. I’m okay with that, though it might be worth explicitly pointing it out for users.
Manually configuring PIDNamespaceTranslation alongside TranslateDescendantPIDs could get tricky—wondering if an auto setting might be cleaner?
Curious to get your thoughts on how this compares to #1657?
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.
Thanks for the review!
Re BTF, is only needed for descendant translation, same-namespace translation still works without it. There's a note in
tracer.Config, but happy to document it somewhere more user-facing if you have a place in mind.RE auto setting, descendant mode needs is only needed in certain scenarios and not sure we should force BTF everywhere (but you might have more context here).
In general, I agree tho that two boolean is a code smell. I've changed that to an enum and renamed it to
PIDNamespaceTranslationModeand documented that the setting is ignored when translation is disabled (ideally we could delete the boolean but I guess we want to be backward compatible).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.
How much overhead would it be if descendant mode is always on, given that BTF and the required BTF descriptions are available? If I understand the eBPF loop correctly, the CPU overhead is negligible (loop early exit).
I am asking because in a mixed/complex environment, the nesting of namespaces is not always obvious and/or it's complex/tedious to configure manually.
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.
If by “always on” you mean enabling it when the BTF fields resolve and otherwise keeping exact translation, that makes sense.
I agree the CPU sampling overhead should be negligible: same-namespace tasks skip the loop, and a direct child only checks two levels. Off-CPU profiling may be different because this runs on scheduler hooks, but we can measure that separately.
We’d still need a strict mode for Kind so missing BTF fails instead of silently dropping child workloads.
Uh oh!
There was an error while loading. Please reload this page.
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've added an
automode with the logic I mentioned above and made it the default. Let me know what you thinkThere 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.
TY, sounds good to me!
Uh oh!
There was an error while loading. Please reload this page.
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.
@florianl TY for asking this Q! #1811 helped me to get some better understanding, so sharing it here.
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 don't think the comparison is accurate for #1801 versus #1657.
Both address the same Kind topology: the profiler runs in the node container's PID namespace and workloads run in descendant namespaces.
#1657 selects a PID using the profiler's namespace depth. However, sibling Kind nodes can exist at the same depth, and a PID from one sibling is not valid in another.
#1801 matches the actual profiler namespace, translates both PID and TID, and rejects sibling namespaces.
So #1801 is functionally the same as #1657 just without the sibling-attribution issue.