fix(otlp): obfuscate malformed json queries and keep key order - #2601
Conversation
Binary Size Analysis (Agent Data Plane)Baseline: c2dbd79 · Comparison: 0cd5c42 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
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 ( |
webern
left a comment
There was a problem hiding this comment.
AI
Self-review found three SQL-obfuscation edge cases. They are pre-existing or intentional differences and do not appear to block this scanner change. Details are inline.
## 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 `thiserror` as a dependency. This is idiomatic and seems like a good call. The scanner's new const fns are `clippy::missing_const_for_fn`, which the workspace enables; they have no runtime effect. `Scanner::new` becoming 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. `JsonObfuscatorConfig` now 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) -> String` transformer 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 return `SQL_OBFUSCATION_FAILURE_REPLACEMENT`. `JsonObfuscationScratch` retains capacity from the largest prior input until the caller trims or drops it. `retained_capacity` and `trim_to` make that policy explicit. ## Rebase onto #2490 #2490 moved `SqlObfuscateConfig`, `SqlObfuscationMode` and `DbmsKind` from `sql` into `obfuscation_config` and renamed the config struct to `SqlConfig`, and it began deserializing `JsonObfuscator` directly from the Agent's `/info` payload. Resolved as follows: - Adopted `obfuscation_config::{DbmsKind, SqlConfig, SqlObfuscationMode}` everywhere, including the `obfuscate_with` doc example. No type is reintroduced in `sql`. - `JsonObfuscatorConfig` keeps `#[serde(default)]` but not `deny_unknown_fields`: #2490 dropped it deliberately, and an `/info` payload from a newer Agent carries fields this struct has no counterpart for. A test pins that forward compatibility. - #2490's hand-written `PartialEq for JsonObfuscatorConfig`, which existed only to skirt the uncomparable `transformer` field, is replaced by a derive now that the field is gone. The config also derives `Eq`. - `obfuscate_resource_for_stats` and `obfuscate_pb_span` now consume the `Result` from `obfuscate_sql`: a resource that obfuscates to nothing is left as sent rather than blanked. ## Agent `/info` field names Checked against the Agent rather than guessed. `pkg/trace/api/info.go` serves a *reduced* view: ```go type reducedJSONObfuscationConfig struct { Enabled bool `json:"enabled"` KeepKeys []string `json:"keep_keys"` } ... oconf.Elasticsearch = reducedJSONObfuscationConfig{Enabled: o.ES.Enabled, KeepKeys: o.ES.KeepValues} ``` So `/info` sends `keep_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 sets `keep_values` and `obfuscate_sql_values`. Rather than rename the Rust fields — `transform_keys` is no longer SQL-specific here, so `obfuscate_sql_values` would 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_names` pins all of it. BREAKING CHANGE: `JsonObfuscatorConfig::transformer` and `JsonStringTransformer` are removed. JSON transforms move to `JsonObfuscator::obfuscate_with` or `JsonObfuscator::obfuscate_into`. SQL obfuscation functions now return `Result`. # How to test the change? The following pass on the rebased branch: ```bash cargo +stable clippy -p libdd-trace-obfuscation --all-targets -- -D warnings cargo +nightly-2026-07-26 fmt --all -- --check cargo nextest run -p libdd-trace-obfuscation # 387 passed cargo test -p libdd-trace-obfuscation --doc cargo check -p libdd-trace-stats -p libdd-data-pipeline-core \ -p libdd-data-pipeline -p libdd-data-pipeline-ffi --all-targets cargo nextest run -p libdd-data-pipeline -p libdd-trace-stats \ -E '!test(tracing_integration_tests::)' # 253 passed ``` The `tracing_integration_tests::` suite needs Docker and was not run locally. `Cargo.lock` gains only `thiserror`, which `LICENSE-3rdparty.csv` already covers, so no regeneration was needed. `cargo deny check` still reports pre-existing workspace advisory and license-policy failures unrelated to this diff. # References - Closes: #2541 - Related: #2490 - Related: DataDog/saluki#2601 Co-authored-by: matt.briggs <matt.briggs@datadoghq.com>
c50aabe to
aee6e9e
Compare
aee6e9e to
bbc63ba
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbc63bab30
ℹ️ 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".
There was a problem hiding this comment.
bbc63ba to
ddf329e
Compare
JSON obfuscation parsed the whole query into a `serde_json::Value`, rewrote it, and re-serialized. A query that was not valid JSON failed to parse and was returned as sent, so raw values reached the backend, and a valid one came back with its keys alphabetized, which changes the resource string that stats are aggregated on. Obfuscate through the JSON obfuscator in libdatadog instead, which scans in a single pass and copies the characters it keeps. Keys keep their order and their text, and a query that stops parsing is obfuscated up to that point and ends in `...`. Whitespace between tokens is dropped, as the reference implementation does. The scan's `obfuscate_into` entry point takes caller-owned output and scratch buffers, which are held on the obfuscator and reused across calls. SQL obfuscation of a value is passed as a call-scoped callback, so the SQL configuration the transform resolved reaches it, and a value whose SQL obfuscation fails carries the reference implementation's failure message rather than `?`. A result that comes back empty is treated as a failure too, as the reference implementation treats it. The reusable buffers keep the capacity they grow to, so an oversized query would otherwise stay held for the obfuscator's lifetime. Capacity past a retention limit is released before the obfuscator is used again; a query within the limit keeps its reuse. Bumps libdd-trace-obfuscation to a revision with the caller-provided JSON transforms. Refs: #2405
ddf329e to
fd7fff4
Compare
## Human Summary Malformed JSON should not bypass obfuscation. We now use the libdatadog provided algorithm which fixes that problem. ## AI Summary This replaces the `serde_json::Value` rewrite with the JSON obfuscator from libdatadog, a single-pass scan that copies the characters it keeps. Malformed input is obfuscated up to the error and ends in `...` instead of being returned with raw values. Valid input keeps its key order and text; whitespace between tokens is dropped, matching the Agent. SQL obfuscation of a value is passed to the scan as a call-scoped callback, so the SQL configuration this transform resolved reaches it, and a value whose SQL obfuscation fails carries the Agent's failure message rather than `?`. A result that comes back empty is treated as a failure too, as the Agent treats it. The scan's `obfuscate_into` entry point takes caller-owned output and scratch buffers, which are held on the obfuscator and reused across calls; capacity past a retention limit is released after an oversized query so it does not stay held. Two divergences from the Agent remain. A SQL value with JSON-only escapes (`\/`, surrogate pairs) is unescaped before obfuscation rather than leaked; a test pins this. A bare top-level value after a complete document is kept rather than obfuscated (DataDog/libdatadog#2577); the Agent also keeps it after an object document. Tests for both cases assert the obfuscated output and are `#[ignore]`d until that is fixed. Bumps the pinned libdd-trace-obfuscation revision to pick up the caller-provided JSON transforms. ## Change Type - [x] Bug fix ## How did you test this PR? Unit tests cover key order, whitespace, nesting, malformed and truncated input, kept subtrees, SQL transformation, escaped keys, multibyte values, multiple documents, and buffer retention after an oversized query. A property test runs arbitrary strings through the obfuscator. The following checks pass: - `make fmt` - `cargo check --workspace` - `cargo check --workspace --tests` - `make check-clippy` - `make check-docs` - `make check-deny` - `make check-licenses` - `make check-release-notes` - `make check-unused-deps` - `cargo nextest run -p saluki-components trace_obfuscation` (73 tests) ## References - Closes: #2405 - Progresses: #2438 Co-authored-by: matt.briggs <matt.briggs@datadoghq.com> a6ffea2
Human Summary
Malformed JSON should not bypass obfuscation. We now use the libdatadog provided algorithm which fixes that problem.
AI Summary
This replaces the
serde_json::Valuerewrite with the JSON obfuscator from libdatadog, a single-pass scan that copies the characters it keeps. Malformed input is obfuscated up to the error and ends in...instead of being returned with raw values. Valid input keeps its key order and text; whitespace between tokens is dropped, matching the Agent.SQL obfuscation of a value is passed to the scan as a call-scoped callback, so the SQL configuration this transform resolved reaches it, and a value whose SQL obfuscation fails carries the Agent's failure message rather than
?. A result that comes back empty is treated as a failure too, as the Agent treats it. The scan'sobfuscate_intoentry point takes caller-owned output and scratch buffers, which are held on the obfuscator and reused across calls; capacity past a retention limit is released after an oversized query so it does not stay held.Two divergences from the Agent remain. A SQL value with JSON-only escapes (
\/, surrogate pairs) is unescaped before obfuscation rather than leaked; a test pins this. A bare top-level value after a complete document is kept rather than obfuscated (DataDog/libdatadog#2577); the Agent also keeps it after an object document. Tests for both cases assert the obfuscated output and are#[ignore]d until that is fixed.Bumps the pinned libdd-trace-obfuscation revision to pick up the caller-provided JSON transforms.
Change Type
How did you test this PR?
Unit tests cover key order, whitespace, nesting, malformed and truncated input, kept subtrees, SQL transformation, escaped keys, multibyte values, multiple documents, and buffer retention after an oversized query. A property test runs arbitrary strings through the obfuscator.
The following checks pass:
make fmtcargo check --workspacecargo check --workspace --testsmake check-clippymake check-docsmake check-denymake check-licensesmake check-release-notesmake check-unused-depscargo nextest run -p saluki-components trace_obfuscation(73 tests)References