feat(obfuscation)!: make json transforms caller-provided - #2548
Conversation
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: c60e9e5 | Docs | View more details | Give us feedback! |
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
|
b36d1da to
e673c33
Compare
BenchmarksComparisonBenchmark execution time: 2026-09-24 12:30:10 Comparing candidate commit c60e9e5 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 176 metrics, 0 unstable metrics.
|
30e9dc1 to
ec9c1c5
Compare
JsonObfuscatorConfig stored a `fn(&str) -> String` transformer, so a caller with runtime configuration to capture could not use it, and the config was neither serializable nor comparable. Remove the transformer field and the `JsonStringTransformer` alias: the config is plain data now, including `transform_keys`, which is no longer `#[serde(skip)]`, and `PartialEq` is derived rather than written by hand around a field that could not be compared. Now that the config is deserialized straight from the Agent's `/info` payload, accept the Agent's own spellings too: `keep_values` for `keep_keys` and `obfuscate_sql_values` for `transform_keys`. `/info` sends `keep_keys` and does not report the transform set at all, so either spelling parses and neither is required. The transform is passed per call instead, as a method-scoped generic: obfuscate_with(input, |v| ...) allocating convenience obfuscate_into(input, out, scratch, f) caller-owned buffers The callback is `for<'a> FnMut(&'a str) -> Result<Cow<'a, str>, E>`, so it can capture anything, needs no Send/Sync/'static, and can return a borrowed, static or owned replacement with no forced allocation. JsonObfuscationScratch holds the reusable working memory, and reports and trims what it retains so reuse cannot silently pin memory. Failures are reported in two channels through JsonObfuscationReport: a JSON scan error, whose partial output is still usable and ends in `...`, and the errors of individual transforms. Nothing is logged behind the caller's back. obfuscate_sql and its wrappers now return Result. An empty result is a failure, as it is in the Agent, and SQL_OBFUSCATION_FAILURE_REPLACEMENT exposes the Agent's replacement text so callers do not split stats aggregation by inventing their own. Fixes #2541, whose non-ASCII identifier case is now pinned by a test. Also match the Agent's JSON unquoting fallback, which keeps the quotes of a value it cannot unquote rather than stripping them, and hand values that carry no escapes to the callback as slices of the input.
ec9c1c5 to
c25301a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c25301ab49
ℹ️ 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".
Use the Agent's non-parsable marker when a non-empty SQL or Cassandra resource obfuscates to nothing. Apply the same policy to protobuf spans, v0.4 spans, and client-side stats so none of those paths can fall back to forwarding comment text as sent.\n\nKeep the JSON transform failure marker distinct and add regression coverage for whitespace-only and comment-only resources.
Escape quotes, backslashes, and control characters before embedding callback replacements in JSON output. Cover both transform entry points and the SQL quoted-identifier configuration that exposed the invalid output.
Eldolfin
left a comment
There was a problem hiding this comment.
Looks good 👍🏽
One thing I saw repeated multiple times in this PR is references of types and files from the agent repo which I think we should avoid because they will get stale and don't add much value IMO
`Pass` tracked what it was doing with four booleans, a literal range and a depth, so states that cannot occur - a key that is also a transformed value, a wiped value inside a kept subtree, a stale literal range - were representable and had to be guarded against by hand. Rename it to `ParserState` and replace those fields with one `ParserPhase`. Each phase carries only the state it uses: the byte range of the literal in hand, and the `KeptSubtree` the pass sits inside, if any. Keeping outlives the phase that reads one key or value, so a transform key inside a kept subtree still rewrites its own value, as before. The flag assignments scattered through the loop become transitions: `begin_next_token`, `begin_literal`, `finish_key` and a `finish_value` that returns the kept subtree that is still open. The `wiped` flag is now the `AwaitingValue` to `ObfuscatedValue` transition, and the defensive `!transforming` guard at a key is satisfied by the phase itself. No behavior change: the output of every entry point is byte for byte what it was, including the keep and transform precedence and the top-level literal quirks, which gain tests here.
…ibdd-data-pipeline, libdd-li... (#2619) <!-- release-proposal-inputs: {"crates":"libdd-capabilities-impl, libdd-common, libdd-data-pipeline, libdd-library-config, libdd-profiling-heap-allocator, libdd-remote-config, libdd-sampling, libdd-shared-runtime, libdd-telemetry, libdd-tinybytes, libdd-trace-utils","main_start_ref":"","level_overrides":"","bypass_standard_checks":false} --> # Release proposal for libdd-capabilities-impl, libdd-common, libdd-data-pipeline, libdd-library-config, libdd-profiling-heap-allocator, libdd-remote-config, libdd-sampling, libdd-shared-runtime, 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. ###⚠️ Crates left out of this proposal affected by its major bumps These publishable workspace crates are not part of this release but their dependency requirement was rewritten on this branch while their published version still requires the old major. If they are a dependency on your deployment not including them in the release could result in duplicate packages or symbol incompatibility. - `libdd-capabilities-impl` `6.0.0` → `7.0.0` affects: `libdd-crashtracker`, `libdd-live-debugger`, `libdd-tracer-flare` - `libdd-common` `7.0.0` → `8.0.0` affects: `libdd-crashtracker`, `libdd-ffe`, `libdd-http-client`, `libdd-ipc`, `libdd-live-debugger`, `libdd-profiling`, `libdd-tracer-flare` - `libdd-data-pipeline` `11.0.0` → `12.0.0` affects: `libdd-live-debugger` - `libdd-remote-config` `6.0.0` → `7.0.0` affects: `libdd-ffe`, `libdd-live-debugger`, `libdd-tracer-flare` - `libdd-telemetry` `9.0.0` → `10.0.0` affects: `libdd-crashtracker` - `libdd-trace-stats` `10.0.0` → `11.0.0` affects: `libdd-ipc` - `libdd-trace-utils` `13.0.0` → `14.0.0` affects: `libdd-tracer-flare` ## libdd-capabilities **Next version:** `4.0.1` **Semver bump:** `patch` **Tag:** `libdd-capabilities-v4.0.1` ### Commits - build: Update workspace to Rust 2024 edition (#2575) ## libdd-common **Next version:** `8.0.0` **Semver bump:** `major` **Tag:** `libdd-common-v8.0.0` ### Commits - fix(ipc)!: use atomic deadlines for shared limiters (#2604) - feat(sidecar)!: Authenticate sidecar connections and shared memory (#2551) - build: Update workspace to Rust 2024 edition (#2575) - feat(trace_utils)!: add mutable metadata (#2545) ## libdd-ddsketch **Next version:** `1.1.3` **Semver bump:** `patch` **Tag:** `libdd-ddsketch-v1.1.3` ### Commits - build: Update workspace to Rust 2024 edition (#2575) ## libdd-profiling-heap-sampler **Next version:** `1.1.1` **Semver bump:** `patch` **Tag:** `libdd-profiling-heap-sampler-v1.1.1` ### Commits - build: Update workspace to Rust 2024 edition (#2575) ## libdd-tinybytes **Next version:** `1.1.5` **Semver bump:** `patch` **Tag:** `libdd-tinybytes-v1.1.5` ### Commits - build: Update workspace to Rust 2024 edition (#2575) ## libdd-trace-protobuf **Next version:** `5.1.0` **Semver bump:** `minor` **Tag:** `libdd-trace-protobuf-v5.1.0` ### Commits - fix(data-pipeline)!: revert changes that made /info un-parsable (#2586) - build: Update workspace to Rust 2024 edition (#2575) ## libdd-capabilities-impl **Next version:** `7.0.0` **Semver bump:** `major` **Tag:** `libdd-capabilities-impl-v7.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^7.0.0 → ^8.0.0 ### Commits - feat(sidecar)!: Authenticate sidecar connections and shared memory (#2551) - build: Update workspace to Rust 2024 edition (#2575) ## libdd-profiling-heap-allocator **Next version:** `1.2.0` **Semver bump:** `minor` **Tag:** `libdd-profiling-heap-allocator-v1.2.0` ### Commits - build: Update workspace to Rust 2024 edition (#2575) ## libdd-library-config **Next version:** `4.2.0` **Semver bump:** `minor` **Tag:** `libdd-library-config-v4.2.0` ### Commits - build: Update workspace to Rust 2024 edition (#2575) ## libdd-trace-normalization **Next version:** `4.2.0` **Semver bump:** `minor` **Tag:** `libdd-trace-normalization-v4.2.0` ### Commits - build: Update workspace to Rust 2024 edition (#2575) ## libdd-remote-config **Next version:** `7.0.0` **Semver bump:** `major` **Tag:** `libdd-remote-config-v7.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^7.0.0 → ^8.0.0 ### Commits - feat(sidecar)!: Authenticate sidecar connections and shared memory (#2551) - build: Update workspace to Rust 2024 edition (#2575) ## libdd-shared-runtime **Next version:** `6.0.0` **Semver bump:** `major` **Tag:** `libdd-shared-runtime-v6.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^7.0.0 → ^8.0.0 ### Commits - build: Update workspace to Rust 2024 edition (#2575) ## libdd-trace-utils **Next version:** `14.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-utils-v14.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^7.0.0 → ^8.0.0 ### Commits - feat(trace-utils): add _dd.sdk.otlp_export and datadog.sdk.semantics OTLP resource attributes (#2603) - build: Update workspace to Rust 2024 edition (#2575) - feat(trace_utils)!: add mutable metadata (#2545) ## libdd-dogstatsd-client **Next version:** `8.0.0` **Semver bump:** `major` **Tag:** `libdd-dogstatsd-client-v8.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^7.0.0 → ^8.0.0 ### Commits - feat(sidecar)!: Authenticate sidecar connections and shared memory (#2551) - build: Update workspace to Rust 2024 edition (#2575) ## libdd-telemetry **Next version:** `10.0.0` **Semver bump:** `major` **Tag:** `libdd-telemetry-v10.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^7.0.0 → ^8.0.0 ### Commits - build: Update workspace to Rust 2024 edition (#2575) - feat(telemetry)!: Use mutable metadata (#2552) ## libdd-sampling **Next version:** `8.0.0` **Semver bump:** `major` **Tag:** `libdd-sampling-v8.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^7.0.0 → ^8.0.0 - `libdd-trace-utils`: ^13.0.0 → ^14.0.0 ### Commits - build: Update workspace to Rust 2024 edition (#2575) ## libdd-trace-obfuscation **Next version:** `10.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-obfuscation-v10.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^7.0.0 → ^8.0.0 - `libdd-trace-utils`: ^13.0.0 → ^14.0.0 ### Commits - fix(data-pipeline)!: revert changes that made /info un-parsable (#2586) - build: Update workspace to Rust 2024 edition (#2575) - feat(obfuscation)!: make json transforms caller-provided (#2548) ## libdd-trace-stats **Next version:** `11.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-stats-v11.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^7.0.0 → ^8.0.0 - `libdd-telemetry`: ^9.0.0 → ^10.0.0 - `libdd-trace-obfuscation`: ^9.0.0 → ^10.0.0 - `libdd-trace-utils`: ^13.0.0 → ^14.0.0 ### Commits - feat(sidecar)!: Authenticate sidecar connections and shared memory (#2551) - build: Update workspace to Rust 2024 edition (#2575) - feat(trace_utils)!: add mutable metadata (#2545) - fix(stats): fix precedence for http endpoint (#2582) ## libdd-data-pipeline-core **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-data-pipeline-core-v3.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^7.0.0 → ^8.0.0 - `libdd-trace-obfuscation`: ^9.0.0 → ^10.0.0 - `libdd-trace-stats`: ^10.0.0 → ^11.0.0 - `libdd-trace-utils`: ^13.0.0 → ^14.0.0 ### Commits - build: Update workspace to Rust 2024 edition (#2575) - feat(trace_utils)!: add mutable metadata (#2545) ## libdd-data-pipeline **Next version:** `12.0.0` **Semver bump:** `major` **Tag:** `libdd-data-pipeline-v12.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^7.0.0 → ^8.0.0 - `libdd-telemetry`: ^9.0.0 → ^10.0.0 - `libdd-trace-obfuscation`: ^9.0.0 → ^10.0.0 - `libdd-trace-stats`: ^10.0.0 → ^11.0.0 - `libdd-trace-utils`: ^13.0.0 → ^14.0.0 ### Commits - feat(trace-utils): add _dd.sdk.otlp_export and datadog.sdk.semantics OTLP resource attributes (#2603) - fix(data-pipeline)!: revert changes that made /info un-parsable (#2586) - build: Update workspace to Rust 2024 edition (#2575) - feat(telemetry)!: Use mutable metadata (#2552) - feat(trace_utils)!: add mutable metadata (#2545) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: iunanua <18325288+iunanua@users.noreply.github.com>
Human Summary
While working on DataDog/saluki#2601, I discovered that I couldn’t use libdatadog’s JSON obfuscator as written. Its transformer was a plain function pointer whose only argument was the JSON value. Unlike a closure, a function pointer cannot capture runtime state, so there was no way to pass Saluki’s dynamically loaded SQL obfuscation configuration without putting it in static or global state. Existing users can configure ordinary SQL obfuscation, but the existing JSON transformer API provides no normal way to pass those SQL settings when transforming SQL embedded in JSON. The provided transformer uses default settings.
In fixing this, I decided to take it a little further. Transform failures are now returned as structured information, scan failures use a typed error, and the hot-path API accepts caller-owned output and scratch buffers so repeated calls can reuse their allocations. Transformations return
Cow<str>, which also avoids forcing an allocation when they can return borrowed text.The coding agent brought in
thiserroras a dependency. This is idiomatic and seems like a good call. The scanner's new const fns areclippy::missing_const_for_fn, which the workspace enables; they have no runtime effect.Scanner::newbecoming restart is for buffer reuse. The parse-state stack now survives between passes.What does this PR do?
This separates JSON obfuscation policy from transformation behavior.
JsonObfuscatorConfignow contains only serializable, comparable data. Callers provide transforms per call through generic closures that can capture runtime SQL configuration.The allocating API is joined by
obfuscate_into, which reuses caller-owned output and scratch buffers. Scratch capacity is observable and explicitly trimmable so callers can choose their memory-retention policy.JSON scan errors and transform errors are reported separately. SQL obfuscation now reports an empty result as
SqlObfuscationError::EmptyResult, and the crate exports the Agent's exact SQL failure replacement. This also pins non-ASCII identifier behavior and fixes the JSON unquoting fallback to retain the literal when unquoting fails.Motivation
The stored
fn(&str) -> Stringtransformer cannot capture runtime configuration or report errors. This prevents Agent-compatible configured SQL transformation inside JSON values.DataDog/saluki#2601 currently duplicates libdatadog's JSON scanner to work around that API. A call-scoped closure lets Saluki use the shared scanner while retaining its configured SQL behavior, logging, and failure policy.
Additional Notes
Transform callbacks return
Cow<str>, allowing borrowed, static, or owned replacements. The callback is generic and allocation-free; no transformer trait or stored trait object is introduced.A transform error replaces that value with
"?", records the error, and continues. Callers that need the Agent's SQL fallback can observe and log the SQL error inside the callback, then returnSQL_OBFUSCATION_FAILURE_REPLACEMENT.JsonObfuscationScratchretains capacity from the largest prior input until the caller trims or drops it.retained_capacityandtrim_tomake that policy explicit.Rebase onto #2490
#2490 moved
SqlObfuscateConfig,SqlObfuscationModeandDbmsKindfromsqlintoobfuscation_configand renamed the config struct toSqlConfig, and it began deserializingJsonObfuscatordirectly from the Agent's/infopayload. Resolved as follows:obfuscation_config::{DbmsKind, SqlConfig, SqlObfuscationMode}everywhere, including theobfuscate_withdoc example. No type is reintroduced insql.JsonObfuscatorConfigkeeps#[serde(default)]but notdeny_unknown_fields: feat(data-pipeline)!: refactor agent's /info obfuscation config format #2490 dropped it deliberately, and an/infopayload from a newer Agent carries fields this struct has no counterpart for. A test pins that forward compatibility.PartialEq for JsonObfuscatorConfig, which existed only to skirt the uncomparabletransformerfield, is replaced by a derive now that the field is gone. The config also derivesEq.obfuscate_resource_for_statsandobfuscate_pb_spannow consume theResultfromobfuscate_sql: a resource that obfuscates to nothing is left as sent rather than blanked.Agent
/infofield namesChecked against the Agent rather than guessed.
pkg/trace/api/info.goserves a reduced view:So
/infosendskeep_keys, which is already this crate's field name, and it does not report the transform set at all. The Agent's own obfuscation config (pkg/obfuscate,apm_config.obfuscation.*) calls the same two setskeep_valuesandobfuscate_sql_values. Rather than rename the Rust fields —transform_keysis no longer SQL-specific here, soobfuscate_sql_valueswould be a lie — both Agent spellings are accepted as#[serde(alias = ...)], in the same style as #2490's PascalCase aliases.obfuscation_config::tests::test_agent_json_obfuscation_field_namespins all of it.BREAKING CHANGE:
JsonObfuscatorConfig::transformerandJsonStringTransformerare removed. JSON transforms move toJsonObfuscator::obfuscate_withorJsonObfuscator::obfuscate_into. SQL obfuscation functions now returnResult.How to test the change?
The following pass on the rebased branch:
The
tracing_integration_tests::suite needs Docker and was not run locally.Cargo.lockgains onlythiserror, whichLICENSE-3rdparty.csvalready covers, so no regeneration was needed.cargo deny checkstill reports pre-existing workspace advisory and license-policy failures unrelated to this diff.References