feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode#2061
Conversation
b0f4da8 to
50536ba
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13d5a971de
ℹ️ 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".
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. |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 7fc9f4c | Docs | Datadog PR Page | Give us feedback! |
64603e7 to
4e49301
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e49301fa3
ℹ️ 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".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2061 +/- ##
==========================================
+ Coverage 73.47% 73.57% +0.09%
==========================================
Files 475 477 +2
Lines 78999 79219 +220
==========================================
+ Hits 58048 58283 +235
+ Misses 20951 20936 -15
🚀 New features to boost your workflow:
|
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
|
|
It looks like the description is a bit off mentioning fixes and bugs that seem to be coming from previous commits of the branch and not from the current state of the feature |
I tried letting a cheap AI read the commits and make a description, I don't think it's too valuable and can even be detrimental, as it were. |
|
I rewrote it cleaner and straighter to the point |
There was a problem hiding this comment.
The code LGTM overall, but I'm not entirely sure to fully grasp in which context (sync or async) is this API supposed to be consumed. In particular we use standard sync thread mechanisms (condvars, mutexes) in an async context, which could be problematic (even in a multi-thread context, a condvar will block the current executor thread, which is not ideal).
On a different front, I remember @paullegranddc said he would rather spawn our own shared runtime in a separate thread than hooking in the client's runtime. I don't have a strong opinion myself, but I wonder if this discussion had a conclusion.
df92857 to
9d305e7
Compare
9d305e7 to
6b5b4db
Compare
6b5b4db to
5b21367
Compare
|
Since the implementation was overhauled, I'll just resolve past conversations here, as they don't fit the current implementation. |
5b21367 to
61fbe96
Compare
yannham
left a comment
There was a problem hiding this comment.
Left a few remarks but otherwise looks good (only skimmed through the native implementation, as I suppose it's just the original shared runtime moved)👍
ff3c135 to
eb913a1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fffdcbe44
ℹ️ 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".
f292e28 to
10554e2
Compare
890b2ff to
a91e5c7
Compare
VianneyRuhlmann
left a comment
There was a problem hiding this comment.
Few comments regarding the doc but LGTM.
I think this is a real improvement for the sahred runtime semantics, thanks for doing this
dddc831 to
7fc9f4c
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
…ibdd-data-pipeline, libdd-li... (#2201) # Release proposal for libdd-capabilities-impl, libdd-common, libdd-data-pipeline, libdd-library-config, libdd-remote-config, libdd-sampling, libdd-telemetry, libdd-tinybytes, libdd-trace-utils and their dependencies This PR contains version bumps based on public API changes and commits since last release. ## libdd-capabilities **Next version:** `2.1.0` **Semver bump:** `minor` **Tag:** `libdd-capabilities-v2.1.0` ### Commits - feat(data-pipeline)!: add stdout log trace exporter (#2074) ## libdd-common **Next version:** `5.1.0` **Semver bump:** `minor` **Tag:** `libdd-common-v5.1.0` ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - fix: update rustls-webpki to 0.103.13 (#2187) - fix: update anyhow for unsoundness (#2186) - feat(machine id): Add helpers in ddcommon to fetch the machine UUID l… (#2163) ## libdd-ddsketch **Next version:** `1.1.0` **Semver bump:** `minor` **Tag:** `libdd-ddsketch-v1.1.0` ### Commits - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - test(ddsketch): add microbenchmarks for add/encode/collapse (#2125) ## libdd-trace-protobuf **Next version:** `4.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-protobuf-v4.0.0` ### Commits - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(stats)!: add whole key cardinality limit (#2158) - feat(remote-config)!: use the proto file from the agent (#2165) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) ## libdd-capabilities-impl **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-capabilities-impl-v3.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ### Commits - feat(data-pipeline)!: add stdout log trace exporter (#2074) ## libdd-library-config **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-library-config-v3.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - feat(library-config)!: caller-supplied threadlocal schema and extra process-context attributes (#2162) - fix(otel-thread-ctx): put the threadlocal attributes at the right place in the context (#2167) ## libdd-remote-config **Next version:** `2.0.0` **Semver bump:** `major` **Tag:** `libdd-remote-config-v2.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - refactor(libdd-remote-config)!: hide Target inner properties so they are not leaked (#2182) - feat(remote-config)!: use the proto file from the agent (#2165) - refactor(rc): reexport Endpoint and Tag common types (#2147) ## libdd-trace-normalization **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-normalization-v3.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.1 → ^4.0.0 ### Commits - feat(data-pipeline)!: CSS Trace Filters (#1985) ## libdd-shared-runtime **Next version:** `2.0.0` **Semver bump:** `major` **Tag:** `libdd-shared-runtime-v2.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ### Commits - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(shared-runtime)!: use weak waker in trigger [APMSP-3371] (#2050) ## libdd-trace-utils **Next version:** `9.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-utils-v9.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - ci(miri): skip slow miri tests (#2188) - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(data-pipeline): add agentless export (#2081) - feat(data-pipeline)!: add stdout log trace exporter (#2074) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) - feat(otlp)!: Export OTLP spans with attribute-level OTel compatibility (#2091) - test(trace-utils): add V05 msgpack decode microbenchmark (#2127) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - test(trace-utils): add VecMap microbenchmarks (#2126) - chore(stats)!: submit p0 telemetry in stats (#2130) - refactor(change-buffer)!: replace slot index with span_id, fix segment isolation (#2105) - feat(data-pipeline)!: CSS Trace Filters (#1985) - feat(trace-exporter): add v1 span and its encoder (#2039) - fix(trace-utils): mark decoded span maps as deduped (#2110) - feat(trace-utils)!: change buffer implementation (#2055) - feat(native-spans)!: change buffer foundation (#2046) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) - test: fix timeouts on heavily contended scenarios (#2093) ## libdd-telemetry **Next version:** `6.0.0` **Semver bump:** `major` **Tag:** `libdd-telemetry-v6.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 ### Commits - ci(miri): skip slow miri tests (#2188) - refactor(libdd-telemetry)!: avoid leaking libdd-common types in the public API (#2152) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) ## libdd-trace-obfuscation **Next version:** `5.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-obfuscation-v5.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - ci(miri): skip slow miri tests (#2188) - fix: update anyhow for unsoundness (#2186) ## libdd-trace-stats **Next version:** `6.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-stats-v6.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(stats)!: send telemetry for cardinality limits (#2159) - feat(stats)!: add whole key cardinality limit (#2158) - fix(trace-stats)!: add grpc_method to aggregation key (#2151) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) ## libdd-data-pipeline **Next version:** `7.0.0` **Semver bump:** `major` **Tag:** `libdd-data-pipeline-v7.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 - `libdd-telemetry`: ^5.0.1 → ^6.0.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-stats`: ^5.0.0 → ^6.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - feat(trace_exporter): enable telemetry in stats exporter (#2160) - refactor(libdd-telemetry)!: avoid leaking libdd-common types in the public API (#2152) - feat(stats): emit canonical gRPC status name for OTLP rpc.response.status_code (#2183) - feat(data-pipeline): add agentless export (#2081) - feat(stats)!: send telemetry for cardinality limits (#2159) - feat(stats)!: add whole key cardinality limit (#2158) - fix(trace-stats)!: add grpc_method to aggregation key (#2151) - feat(data-pipeline)!: add stdout log trace exporter (#2074) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) - feat(otlp)!: Export OTLP spans with attribute-level OTel compatibility (#2091) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - chore(stats)!: submit p0 telemetry in stats (#2130) - feat(data-pipeline)!: CSS Trace Filters (#1985) - feat(shared-runtime)!: use weak waker in trigger [APMSP-3371] (#2050) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) - feat(stats)!: add endpoint gating to client-side stats [APMSP-3361] (#2040) ## libdd-dogstatsd-client **Next version:** `4.0.0` **Semver bump:** `major` **Tag:** `libdd-dogstatsd-client-v4.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ## libdd-sampling **Next version:** `5.0.0` **Semver bump:** `major` **Tag:** `libdd-sampling-v5.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 [APMSP-3371]: https://datadoghq.atlassian.net/browse/APMSP-3371?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: iunanua <[email protected]>
# What ? Split `SharedRuntime` into a trait with three implementations: - `ForkSafeRuntime` *(native only)*: owns a multi-thread tokio runtime, supports fork hooks (`before_fork` / `after_fork_parent` / `after_fork_child`) and synchronous `shutdown`. - `BasicRuntime` *(native only)*: wraps a library-built or caller-provided `Arc<tokio::runtime::Runtime>`, no fork hooks, no synchronous shutdown. - `LocalRuntime` *(wasm32 only)*: single-threaded executor that spawns workers via `wasm_bindgen_futures::spawn_local`, no fork protocol, async-only. `BlockingRuntime` *(native only)* is a sub-trait of `SharedRuntime` that adds `block_on`; implemented by `ForkSafeRuntime` and `BasicRuntime`. Sync facades on the trace exporter (`send` / `shutdown` / `build`) bound their runtime parameter on it. `TraceExporter` is generic over the runtime — `TraceExporter<C, R: SharedRuntime>` — so callers pick `ForkSafeRuntime`, `BasicRuntime`, or `LocalRuntime` explicitly. The FFI pins `R = ForkSafeRuntime` for ABI stability. # Why ? Some callers already have a tokio runtime they want to reuse instead of letting libdatadog create its own. The previous `SharedRuntime` only supported the fork-safe owned case. Splitting into distinct types makes the lifecycle and fork-safety contract explicit and lets callers pick the model that matches their environment. The wasm32 target previously shared a file with the native implementation behind `#[cfg]` walls; it is now a dedicated module with a clean separation. # How ? - Introduce a `SharedRuntime` trait (`new`, `spawn_worker`, `shutdown_async`). - Add a native-only `BlockingRuntime: SharedRuntime` sub-trait with `block_on`. - Move the existing owned-runtime logic into `ForkSafeRuntime`; fork hooks and sync `shutdown` are inherent methods (not part of the trait). - Add `BasicRuntime::with_worker_threads` (library-built) and `BasicRuntime::from_handle(Arc<Runtime>)` (caller-provided). - Extract the wasm32 `spawn_local` path from `fork_safe.rs` into its own `local.rs` module as `LocalRuntime`. - Make `TraceExporter<C, R>` and `TraceExporterBuilder<R>` generic over the runtime; sync entry points additionally require `R: BlockingRuntime`. `Default` for the builder is impl'd only for `R = ForkSafeRuntime` on native so `TraceExporterBuilder::default()` resolves unambiguously. - `restart_on_fork = true` is silently ignored (with a `warn!`) on `BasicRuntime` and `LocalRuntime` since they do not implement a fork protocol. - Update FFI to pin `R = ForkSafeRuntime` in its `TraceExporter` type alias. # Additional Notes - Breaking change for Rust callers of `libdd_shared_runtime`: `SharedRuntime` is now a trait; use `ForkSafeRuntime::with_worker_threads(1)` (or the trait method `SharedRuntime::new()` with the trait in scope) instead of `SharedRuntime::new()`. - FFI handle type changes from `SharedRuntime` to `ForkSafeRuntime`. Co-authored-by: jules.wiriath <[email protected]> Signed-off-by: Taegyun Kim <[email protected]>
What ?
Split
SharedRuntimeinto a trait with three implementations:ForkSafeRuntime(native only): owns a multi-thread tokio runtime, supports forkhooks (
before_fork/after_fork_parent/after_fork_child) and synchronousshutdown.BasicRuntime(native only): wraps a library-built or caller-providedArc<tokio::runtime::Runtime>, no fork hooks, no synchronous shutdown.LocalRuntime(wasm32 only): single-threaded executor that spawns workers viawasm_bindgen_futures::spawn_local, no fork protocol, async-only.BlockingRuntime(native only) is a sub-trait ofSharedRuntimethat addsblock_on; implemented byForkSafeRuntimeandBasicRuntime. Sync facades on thetrace exporter (
send/shutdown/build) bound their runtime parameter on it.TraceExporteris generic over the runtime —TraceExporter<C, R: SharedRuntime>— so callers pick
ForkSafeRuntime,BasicRuntime, orLocalRuntimeexplicitly. TheFFI pins
R = ForkSafeRuntimefor ABI stability.Why ?
Some callers already have a tokio runtime they want to reuse instead of letting
libdatadog create its own. The previous
SharedRuntimeonly supported thefork-safe owned case. Splitting into distinct types makes the lifecycle and
fork-safety contract explicit and lets callers pick the model that matches their
environment.
The wasm32 target previously shared a file with the native implementation behind
#[cfg]walls; it is now a dedicated module with a clean separation.How ?
SharedRuntimetrait (new,spawn_worker,shutdown_async).BlockingRuntime: SharedRuntimesub-trait withblock_on.ForkSafeRuntime; fork hooks and syncshutdownare inherent methods (not part of the trait).BasicRuntime::with_worker_threads(library-built) andBasicRuntime::from_handle(Arc<Runtime>)(caller-provided).spawn_localpath fromfork_safe.rsinto its ownlocal.rsmodule asLocalRuntime.TraceExporter<C, R>andTraceExporterBuilder<R>generic over the runtime;sync entry points additionally require
R: BlockingRuntime.Defaultfor thebuilder is impl'd only for
R = ForkSafeRuntimeon native soTraceExporterBuilder::default()resolves unambiguously.restart_on_fork = trueis silently ignored (with awarn!) onBasicRuntimeand
LocalRuntimesince they do not implement a fork protocol.R = ForkSafeRuntimein itsTraceExportertype alias.Additional Notes
libdd_shared_runtime:SharedRuntimeisnow a trait; use
ForkSafeRuntime::with_worker_threads(1)(or the trait methodSharedRuntime::new()with the trait in scope) instead ofSharedRuntime::new().SharedRuntimetoForkSafeRuntime.