feat(sidecar)!: support appsec helper-rust integration with sidecar - #2310
Open
cataphract wants to merge 12 commits into
Open
feat(sidecar)!: support appsec helper-rust integration with sidecar#2310cataphract wants to merge 12 commits into
cataphract wants to merge 12 commits into
Conversation
Read Cargo's target OS and family in the crashtracker build script instead of build-host cfg values. Build the CXX bridge for its target, keep Unix-only C support and test libraries off Windows, and select the dynamic CRT for Windows targets. Teach the spawn worker trampoline build to distinguish MSVC and GNU Windows environments, skip Unix libraries, and use compatible C++ flags without promoting MinGW warnings to errors. Use lowercase SDK header names and enable the LibraryLoader API for Windows crashtracker tests.
Add an AppSec backend factory that an embedding application can register from a custom sidecar entry point. Start and stop that backend with the sidecar listener, and replace the dynamically loaded helper library and its private socket configuration with a sidecar request/response RPC. Expose the RPC through the blocking client and C FFI. Associate helper client IDs with sidecar connections, notify the backend when connections or sessions close, and reject stale or conflicting IDs after a restart. Send helper-targeted events to the configured AppSec log.
Teach the IPC service macro to generate a serialize-only client request enum when parameters declare alternate #[ClientType] representations. Add blocking channel calls that serialize borrowed request values. Use byte slices for AppSec session IDs and payloads, and retain one request across transport retries. The server still decodes the existing owned request type, avoiding request allocations in FFI and clones on retry.
Format embedded AppSec helper records with a UTC timestamp, level, message, and module. Normalize records bridged from the log crate before filtering helper targets and rendering their module paths. Write helper records to their configured file and exclude other sidecar records from it. Treat "<sidecar log>" as a request to use the main sidecar log without creating a separate helper log layer.
Expose a client factory to the registered AppSec backend so embedded components can enqueue actions into the sidecar telemetry receiver without using FFI. Bind each client to an instance, service, and environment while allowing its application metadata to be rebound. Refresh cached telemetry clients on lookup so active clients are not expired while the in-process path is in use.
Move AppSec lifecycle management into the sidecar server and add a sidecar request that starts the registered backend on demand. Coordinate concurrent initialization and ensure shutdown is owned by one caller. This lets thread-mode listeners receive AppSec configuration after a client connects, while process-mode sidecars still start from daemon configuration.
Contributor
📚 Documentation Check Results📦
|
Contributor
Clippy Allow Annotation ReportTracked Clippy
By file and crateBy file
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. Panic-inducing macros in particular should be avoided. In the future, this report may become a PR-blocking quality gate. |
cataphract
changed the title
Contributor
🔒 Cargo Deny Results📦
|
Contributor
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: d9d797c | Docs | Datadog PR Page | Give us feedback! |
![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: 7112284f0b
ℹ️ 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".
Contributor
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
BenchmarksComparisonBenchmark execution time: 2026-08-07 15:01:35 Comparing candidate commit d9d797c in PR branch Found 2 performance improvements and 2 performance regressions! Performance is the same for 136 metrics, 0 unstable metrics.
|
bwoebi
reviewed
Aug 6, 2026
bwoebi
reviewed
Aug 6, 2026
| _ = APPSEC_BACKEND_FACTORY.set(factory); | ||
| } | ||
|
|
||
| /// Publishes one AppSec backend and coordinates its one-way lifecycle. |
Contributor
There was a problem hiding this comment.
I find this construction quite peculiar.
What you want, is essentially a trait AppSecManager { ... } with a Arc<tokio::sync::OnceCell<dyn AppSecManager>> with a .get_or_init(async || APPSEC_BACKEND_FACTORY()) (sort of)?
And then have a shutdown function on the trait which expects self, and using Arc::into_inner() in shutdown.
To support no-op (i.e. failed startup) mode, you can just have an impl with trait methods which do nothing / return disconnect.
Like:
// Calling send_message/disconnect is allowed to race with shutdown. It either
// completes successfully or observes a closed channel in the backend and fails,
// returning a message requesting a disconnect.
is just not going to happen anyway - when appsec shutdown happens, all sidecar connections are already shut down.
This is an overly defensive design with unncessary complexity from there.
Contributor
Author
There was a problem hiding this comment.
trait AppSecManager { ... } with a Arc<tokio::sync::OnceCell> with a .get_or_init(async || APPSEC_BACKEND_FACTORY()) (sort of)
Yes, I think so, for the initialization part. Of course, I'd rather not replace
appsec: Option<Arc<AppSecManager>>,
on struct SidecarServer with that, plus expose the factory and force it to use get_or_init. So in practice this would be done in a public encapsulating type inside the appsec.rs that would save callers from it. In fact, I had an earlier version more or less along those lines.
The "peculiarity" (it's just a state machine after all) is all about the shutdown. This is already a bit of a simplified version -- previously I had also synchronization around the shutdown (not just initialization) to ensure no messages were sent to the backend after a shutdown was requested -- which I then replaced with that comment to make it a bit simpler. And if OnceCell can't help all the lifecycle state I need (it only does uninitialized and initialized), and it need to track it in other fields, AND if I need an extra wrapper type... well, at some point this design became (to me) clearer.
Of course, this all hinges on the complexity for shutdown actually being necessary -- for instance, it handles cases where shutdown is called before and during initialization, where shutdown is called more than once, discards messages after shutdown or after failed initialization, etc.
And then have a shutdown function on the trait which expects self, and using Arc::into_inner() in shutdown.
I don't think that this, at least literally, would work. Maybe Mutex<Option<Box<dyn AppSecManager>>>, and then:
pub async fn shutdown(server) {
if let Some(m) = server.appsec.lock().unwrap().take() {
m.shutdown().await; // fn shutdown(self: Box<Self>) -> impl Future
}
}
but then you have the mutex
Contributor
There was a problem hiding this comment.
Of course, this all hinges on the complexity for shutdown actually being necessary -- for instance, it handles cases where shutdown is called before and during initialization, where shutdown is called more than once, discards messages after shutdown or after failed initialization, etc.
Yes, that's basically what I'm saying. I don't think most of that is necessary to handle at all, what's the actual use case? The sidecar as is literally cannot shut down while there are currently actively processed payloads, which initialization depends on.
And no, Arc::into_inner() should just work, it gives you an owned value which you can then just orderly shut down. Because, at least assuming the shutdown in the entrypoint shutdown is the only one, that should work out fine, no?
Contributor
Author
There was a problem hiding this comment.
Arc::into_inner() requires that no other shared owner exist. I don't think that can be guaranteed with all the cloning of SidecarServer.
But I'll otherwise try to simplify this
Contributor
Author
There was a problem hiding this comment.
I've pushed a simplified version in f5a4ba0
bwoebi
reviewed
Aug 6, 2026
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.
See DataDog/dd-trace-php#3725
Also fix zigbuild windows builds, for easier testing of windows when running Linux (zigbuild + wine).