DEBUG-4675 DI: fix off-by-one in serializer max capture depth#5753
Conversation
The Serializer truncation check used `depth < 0`, causing one extra level of nesting to be captured. With `max_capture_depth=N`, this captured N+1 levels (top object plus N nested) instead of N levels. Change the check to `depth <= 0` in the Array, Hash, and Object branches so truncation fires when the next recursion would exceed the configured depth. With the default `max_capture_depth=3`, snapshots now contain 3 levels of nesting, matching the documented behavior and the equivalent behavior in other tracer libraries. System test `tests/debugger/test_debugger_probe_snapshot.py ::Test_Debugger_Line_Probe_Snaphots::test_default_max_reference_depth` covers this case. Existing `serializer_spec.rb` "depth exceeded" cases are updated to reflect the corrected truncation point.
p-datadog
added
the
AI Generated
dd-octo-sts
Bot
added
the
debugger
![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: 7843c90448
ℹ️ 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".
Forces the system-tests assertion covered by DEBUG-4675 to run in this PR's CI, even though manifests/ruby.yml still marks it as `bug (DEBUG-4675)`. Remove this entry before merging — the companion PR DataDog/system-tests#6918 removes the bug marker once merged.
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 36995de | Docs | Datadog PR Page | Give us feedback! |
Address review comment 3234907136:
the AR custom serializer's `depth: depth - 1` was effectively compensating
for the off-by-one this PR fixes in `lib/datadog/di/serializer.rb`. The
synthetic `{attributes:, new_record:}` hash is a transparent representation
of the AR model's fields, not a real additional nesting level — `serialize_value`
already decrements depth as it recurses into Array/Hash/object entries.
With the off-by-one fixed in serializer.rb, the AR decrement now over-
truncates: at the default `max_capture_depth=2`, `:attributes` hits the
new `<= 0` truncation and becomes `notCapturedReason` instead of being
captured down to leaf values.
- Pass `depth: depth` instead of `depth: depth - 1` in lib/datadog/di/contrib/active_record.rb:40
- Reworded the depth guidance in the surrounding doc comment so the
custom-serializer contract no longer prescribes a `- 1` that depended on
the buggy semantics.
The existing AR spec in spec/datadog/di/contrib/active_record/serializer_active_record_spec.rb
exercises this at the default max_capture_depth=2 and expects :attributes
captured down to leaf values; that assertion shape is preserved.
BenchmarksBenchmark execution time: 2026-05-13 15:32:32 Comparing candidate commit 161ab4a in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 45 metrics, 1 unstable metrics.
|
This reverts commit 1392e5e.
Replace the unconditional removal with a `component_version: <2.34.0` gate, matching the pattern used for DEBUG-3747 / DEBUG-4343. The off-by-one bug exists in all released dd-trace-rb versions through 2.33.0; the fix in DataDog/dd-trace-rb#5753 lands on master at 2.34.0. For tracer versions <2.34.0 the bug marker still applies. For 2.34.0+ the test runs and passes (force-run validated this on rails72/rails80/ uds-rails End-to-end CI in dd-trace-rb#5753, commit 161ab4a373).
3 participants
What does this PR do?
Fixes an off-by-one bug in the Dynamic Instrumentation snapshot serializer: with
max_capture_depth=N, the serializer was capturing N+1 levels of nested objects (the top-level value plus N nested levels) instead of N levels.The truncation check is in three branches of
Serializer#serialize_value—Array,Hash, and the catch-all object branch. Each usedif depth < 0, which only fires after one extra recursion. Changed toif depth <= 0so truncation fires when the next recursion would exceed the configured depth.Motivation:
System test
Test_Debugger_Line_Probe_Snaphots::test_default_max_reference_depth(added in DataDog/system-tests#5624) is filed against Ruby asbug (DEBUG-4675)with the note "Ruby has off-by-one bug: captures 4 levels instead of 3". The test setsmax_capture_depth=3(default) and asserts exactly 3 levels of nesting are captured before truncation.The previous behavior was inconsistent with the default-depth documentation and with other Datadog tracer libraries.
Change log entry
Yes. Dynamic Instrumentation: Fix off-by-one in
max_capture_depthso snapshots respect the configured nesting limit exactly (previously captured one extra level).Additional Notes:
Three existing "depth exceeded" cases in
spec/datadog/di/serializer_spec.rbtested the buggy behavior (inputs nested one level deeper thanmax_capture_depth=2). Inputs are reduced by one level so the truncation hits the typed value (Array/Hash/Object) the test name promises.The serializer test setup in
spec/datadog/di/serializer_helper.rbusesmax_capture_depth=2. Other DI specs that mockmax_capture_depth=2(instrumenter_spec,probe_notification_builder_spec,integration/probe_notification_builder_spec) only exercise objects with at most one level of nesting and remain unaffected.Companion system-tests PR (draft) removing the bug marker: DataDog/system-tests#6918.
System-tests validation result:
A force-run entry was added to
.github/forced-tests-list.cfgto exercisetest_default_max_reference_depthin this PR's End-to-end CI runs (commit1392e5ecd8). Results on commit161ab4a373:rails72: 21/21 SUCCESSrails80: 21/21 SUCCESSuds-rails: 22/22 SUCCESSThe forced test passed on every End-to-end run against the manifest-supported weblogs (the three the test's parent declaration is scoped to). The force-list format has no weblog scoping, so the entry also ran the test against weblogs the manifest marks
irrelevant(rack, sinatra14, sinatra22/32/41, rails42, rails52, rails61, uds-sinatra) — those failed atFailed to get /debugger/init: expected status code: 200, actual status code: 404because those weblogs do not have the debugger controller endpoints. Those failures are not fix-related.The force entry has been reverted in commit
36995dec85now that the supported-weblog evidence is captured. Going forward, the test re-activates through the companion system-tests PR DataDog/system-tests#6918.How to test the change?
bundle exec rake test:di— the three updated "depth exceeded" cases inserializer_spec.rbcover the new truncation point forArray,Hash, andObject../run.sh DEBUGGER_PROBES_SNAPSHOT --library ruby— the supported-weblog runs in this PR's CI (links above) show the forced test passing on each.