refactor(sidecar)!: Avoid a dedicated socket for crashtracker#2179
Open
bwoebi wants to merge 2 commits into
Open
refactor(sidecar)!: Avoid a dedicated socket for crashtracker#2179bwoebi wants to merge 2 commits into
bwoebi wants to merge 2 commits into
Conversation
Contributor
|
Contributor
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
Contributor
📚 Documentation Check Results📦
|
Contributor
🔒 Cargo Deny Results📦
|
bwoebi
force-pushed
the
bob/sidecar-crashtracker-ipc
branch
from
78bf47c to
f4d9ba8
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78bf47c88d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bwoebi
force-pushed
the
bob/sidecar-crashtracker-ipc
branch
2 times, most recently
from
eaf749d to
15ae3e8
Compare
bwoebi
changed the title
bwoebi
force-pushed
the
bob/sidecar-crashtracker-ipc
branch
3 times, most recently
from
61b955f to
d3bc905
Compare
This allows us doing an unified authentication of the socket on the sidecar side, before handing off to crashtracker. Usually the parent process would be the one connecting to the crashtracker, here it is not. This will allow custom handling, and one less endpoint to secure. The strategy chosen is upgrading to the crashtracker once an IPC socket is received. Currently we recreate a new socket, but we might also decide to reuse the existing IPC connection in future. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
bwoebi
force-pushed
the
bob/sidecar-crashtracker-ipc
branch
from
d3bc905 to
99cefc7
Compare
gyuheon0h
reviewed
Jul 1, 2026
|
|
||
| impl ConnectionSidecarHandler { | ||
| fn new(server: SidecarServer) -> Self { | ||
| fn new(server: SidecarServer, connection: OwnedServerConn) -> Option<Self> { |
Contributor
There was a problem hiding this comment.
This always returns Some(..), right? Is the Option misleading then?
Contributor
Author
There was a problem hiding this comment.
You're right, that code went through two iterations :-D
gyuheon0h
reviewed
Jul 1, 2026
| }; | ||
|
|
||
| // One recv() returns one datagram; a max-sized buffer avoids truncation. | ||
| let read_result = guard.try_io(|inner| { |
Contributor
There was a problem hiding this comment.
Every single poll read allocates and deallocs 4MiB. Is this okay?
Contributor
Author
There was a problem hiding this comment.
Not really a big problem for crashtracker, but yeah, I should move it out of the loop to save some syscalls.
gyuheon0h
reviewed
Jul 1, 2026
| use tokio::io::unix::AsyncFd; | ||
| use tokio::io::{AsyncReadExt, BufReader}; | ||
|
|
||
| fn dgram_pair() -> (OwnedFd, OwnedFd) { |
Contributor
There was a problem hiding this comment.
This test use SOCK_DGRAM but production is SOCK_SEQPACKET. Can we make tests consistent with our prod code or is this intentional?
gyuheon0h
approved these changes
Jul 1, 2026
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This allows us doing an unified authentication of the socket on the sidecar side, before handing off to crashtracker. Usually the parent process would be the one connecting to the crashtracker, here it is not. This will allow custom handling, and one less endpoint to secure.
The strategy chosen is upgrading to the crashtracker once an IPC socket is received. Currently we recreate a new socket, but we might also decide to reuse the existing IPC connection in future.
The changes to the crashtracker crate itself are kept minimal, just adding a new config for a custom socket connector, and using the existing one as default.
There's also a bit of refactoring on the sidecar side, to make it easier to take ownership of the asyncfd.