chore(components, core, io): cutover Datadog Traces encoder and Checks IPC source to full supervision - #2342
chore(components, core, io): cutover Datadog Traces encoder and Checks IPC source to full supervision#2342tobz wants to merge 2 commits into
Conversation
…s IPC source to full supervision
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Binary Size Analysis (Agent Data Plane)Baseline: 8d372a5 · Comparison: 7d7db10 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d7db102ca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .global_thread_pool() | ||
| .spawn_traced_named("checks-ipc-grpc-server", grpc_server.serve(grpc_socket_addr)); | ||
| .spawner() | ||
| .supervisable(grpc_server) |
There was a problem hiding this comment.
Keep Checks IPC gRPC on the worker pool
When Checks IPC receives large or continuous payloads, this now runs the Tonic server and its request handlers on the component supervisor's runtime because supervisable defaults to that runtime. The previous implementation explicitly spawned Server::serve on global_thread_pool(), so this cutover moves protobuf decoding and check_data_to_event processing onto the runtime that drives topology supervision and I/O, allowing Checks traffic to increase scheduling latency for unrelated components. Add .on_worker_pool() before spawning to preserve the previous isolation.
Useful? React with 👍 / 👎.
| /// [`noninterruptible`][Self::noninterruptible] instead. | ||
| /// | ||
| /// # Errors | ||
| /// Interruptible tasks are set to permanently restart by default. |
There was a problem hiding this comment.
Correct the interruptible task restart documentation
This public API documentation says interruptible tasks restart permanently, but interruptible constructs a OneShot builder and ChildBuilder::one_shot uses ChildSpecification::one_shot_worker, which sets RestartType::Temporary. Callers relying on the stated policy could expect a failed background task to recover when it will instead disappear permanently, so document the actual temporary behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The new gRPC primitive makes saluki-io directly use tonic’s router/server APIs without enabling those features. Workspace feature unification can hide the defect, but package-scoped and downstream builds of saluki-io will fail to compile.
🤖 Datadog Autotest · Commit 7d7db10 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| use tonic::{ | ||
| body::Body, | ||
| server::NamedService, | ||
| service::Routes, |
There was a problem hiding this comment.
Declare tonic server features in
saluki-io
Consumers that build saluki-io without saluki-components cannot compile the crate.
Assertion details
- Input: A package-scoped or downstream build where no other dependency enables tonic's
routerandserverfeatures. - Expected: Update
lib/saluki-io/Cargo.tomlto enable tonic'srouterandserverfeatures directly, matching the crate's API usage. This requires a manifest change outside the relevant diff hunk, so no inline suggestion is safe. - Actual: The new module imports
tonic::server::NamedService,tonic::service::Routes, and transport server APIs, butsaluki-iodeclarestonic = { workspace = true }while workspace defaults are disabled. Workspace-wide builds can mask this throughsaluki-components, which enablesrouterandserver, but package-scoped or downstream builds cannot resolve these APIs.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |

Summary
This PR is the first of many (hopefully just a few) for converting components to spawn all of their child tasks under their dedicated supervisor to fully unify task execution in Saluki under the supervision tree model.
This PR is meant to get our feet wet: a simple component (Datadog Traces encoder) and a more complex one (Checks IPC source).
The Datadog Traces encoder conversion exercises spawning a worker based purely on an async function (which is the "simple" model) as well as executing it on the worker pool, rather than whatever runtime the component supervisor is attached to.
The Checks IPC source conversion exercises spawning a supervisable worker, which is a new addition in this PR: I realized that we're not always going to want to just spawn async fns that can't ever be restarted, and that it was a good opportunity to update
ComponentSpawnerto support that and add a new gRPC server primitive which implementsSupervisablenatively so we could exercise the new support for spawningSupervisables.Change Type
How did you test this PR?
Existing tests.
References
DADP-2