DEBUG-5107 DEBUG-5111 DI: tolerant probe path matching#5754
Conversation
…be path matching Probe source paths are sometimes delivered by IDE tooling that does not match the on-disk casing (DEBUG-5107) or uses Windows-style backslash separators (DEBUG-5111). The line probe path matcher previously rejected both, so probes never moved past RECEIVED to INSTALLED in those cases. In `Utils.path_matches_suffix?`, downcase both the actual file path and the user-supplied suffix, and translate backslashes in the suffix to forward slashes before comparing. In `Utils.path_can_match_spec?`, normalize backslashes in the spec up-front so the "/+" regex used to peel off leading directory components keeps working when the user supplies a Windows-style path. The unconditional bug markers for `Test_Debugger_Line_Probe_Statuses::test_probe_status_log_line_with_different_casing` and `::test_probe_status_log_line_with_windows_path` in DataDog/system-tests `manifests/ruby.yml` can be removed once this lands.
Forces `Test_Debugger_Line_Probe_Statuses::test_probe_status_log_line_with_different_casing` and `::test_probe_status_log_line_with_windows_path` to run in this PR's CI even though `manifests/ruby.yml` still marks both as `bug (DEBUG-5107)` / `bug (DEBUG-5111)`. Remove these entries before merging — the companion system-tests PR removes the bug markers.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25989b0658
ℹ️ 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".
| path = path.downcase | ||
| suffix = suffix.tr('\\', '/').downcase |
There was a problem hiding this comment.
Prefer case-sensitive suffix matches first
When a case-sensitive deployment contains two known files that differ only by case, e.g. /app/foo.rb and /app/FOO.rb, this unconditional downcase makes a correctly cased relative probe like foo.rb match both paths. CodeTracker#iseqs_for_path_suffix and #resolve_path_suffix raise MultiplePathsMatch when more than one path matches, so probes that were previously uniquely targetable become ambiguous; the design comment in this file also describes case-insensitive matching as a fallback after case-sensitive attempts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — that's a real regression on case-sensitive deployments where two files differ only by case (e.g. /app/foo.rb and /app/FOO.rb). The design comment in this file (steps 5–8) calls for case-sensitive matching first and case-insensitive as a fallback, which the unconditional downcase short-circuited.
Fixed in 6745b6e by adding a case_insensitive: keyword to Utils.path_matches_suffix? (defaulting to case-sensitive, with backslash normalization still unconditional), and threading two passes through the callers:
- lib/datadog/di/utils.rb —
path_matches_suffix?only downcases whencase_insensitive: true;path_can_match_spec?runs the suffix-shortening loop twice, case-sensitive first. - lib/datadog/di/code_tracker.rb —
iseqs_for_path_suffixandresolve_path_suffixeach run their loop case-sensitive first, falling back to case-insensitive only when no case-sensitive match was found. - lib/datadog/di/instrumenter.rb — case-sensitive
$LOADED_FEATURES.findfirst, case-insensitive fallback. - sig/datadog/di/utils.rbs — updated signature.
- spec/datadog/di/utils_spec.rb — split table-driven cases between case-sensitive default behavior and the
case_insensitive: trueparameter; added case-mismatch cases that assert non-match under the default. - spec/datadog/di/code_tracker_spec.rb — added
#iseqs_for_path_suffix with case-different known pathscovering unique match on correctly-cased lookup, fallback when no case-sensitive match exists, andMultiplePathsMatchwhen the fallback is ambiguous.
| tests/debugger/test_debugger_probe_status.py::Test_Debugger_Line_Probe_Statuses::test_probe_status_log_line_with_different_casing | ||
| tests/debugger/test_debugger_probe_status.py::Test_Debugger_Line_Probe_Statuses::test_probe_status_log_line_with_windows_path |
There was a problem hiding this comment.
Remove temporary forced system tests before merging
As committed, these node ids stay in .github/forced-tests-list.cfg, so every branch after merge will force these debugger status system tests regardless of the system-tests manifest or decorators. The adjacent comment says they are only for the draft companion PR and must be removed before merging; leaving them changes CI test selection after the manifest bug markers are removed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Looked into this — the cleanup is captured in the PR description's "forced-tests-list.cfg edits live in their own commit (25989b0658, titled "DO NOT MERGE"), and the plan is to revert that commit once the companion system-tests PR DataDog/system-tests#6920 lands. Keeping them here for now so this PR's DEBUGGER_PROBES_STATUS job continues to exercise the fix against the bug-marked tests.
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: bf3fc69 | Docs | Datadog PR Page | Give us feedback! |
| # Normalize Windows-style backslash separators (DEBUG-5111) so the | ||
| # suffix-shortening loop's "/+" regex can strip leading components. | ||
| spec = spec.tr('\\', '/') |
There was a problem hiding this comment.
This module is utils, does it make sense to have a method for windows path normalization instead?
There was a problem hiding this comment.
Yep, agreed — there are two tr('\\', '/') call sites now, and a named method makes the intent (interpret Windows-style separators) explicit. Extracted into Utils.normalize_windows_separators in 36584cf and updated both path_matches_suffix? and path_can_match_spec? to use it.
- lib/datadog/di/utils.rb — new
normalize_windows_separatorsmodule function;path_matches_suffix?andpath_can_match_spec?use it. - sig/datadog/di/utils.rbs — added signature.
Address review comment on PR #5754: the previous unconditional downcase in path_matches_suffix? made matching always case-insensitive, which contradicted the design comment in the same file (steps 5-6 case-sensitive, steps 7-8 case-insensitive fallback). On case-sensitive deployments with two files differing only by case (e.g. /app/foo.rb and /app/FOO.rb), a correctly-cased probe path like foo.rb would match both, surfacing as MultiplePathsMatch — a regression vs the pre-PR behavior. Restore the documented ordering: - Utils.path_matches_suffix? gains case_insensitive: keyword (defaults to case-sensitive); backslash normalization remains unconditional. - Utils.path_can_match_spec? runs the suffix-shortening loop twice, case-sensitive first. - CodeTracker#iseqs_for_path_suffix and #resolve_path_suffix run two passes, case-sensitive first, case-insensitive fallback. - Instrumenter#raise_if_probe_in_loaded_features tries $LOADED_FEATURES.find case-sensitive first, case-insensitive fallback. Specs: - spec/datadog/di/utils_spec.rb covers case-sensitive default behavior and the case_insensitive: true parameter explicitly. - spec/datadog/di/code_tracker_spec.rb covers the case-different-paths scenario directly: unique match on correctly-cased lookup, fallback when no case-sensitive match exists, MultiplePathsMatch when the fallback is ambiguous. Verified: standardrb clean, steep clean, full spec/datadog/di/utils_spec.rb, spec/datadog/di/code_tracker_spec.rb, spec/datadog/di/instrumenter_spec.rb, spec/datadog/di/probe_spec.rb pass (200 examples). Co-Authored-By: Claude <[email protected]>
Address review comment on PR #5754: the backslash-to-forward-slash translation was duplicated inline in path_matches_suffix? and path_can_match_spec?. Extracting a named utility method centralizes the operation and makes the intent (interpret Windows-style separators) explicit. Co-Authored-By: Claude <[email protected]>
BenchmarksBenchmark execution time: 2026-05-13 19:09:02 Comparing candidate commit bf3fc69 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 44 metrics, 1 unstable metrics.
|
The DEBUG-5111 Windows path fix was incomplete: Utils.path_matches_suffix?
normalizes backslashes internally, but CodeTracker#iseqs_for_path_suffix
and #resolve_path_suffix have their own suffix-shortening loops that check
working_suffix.include?('/') and strip via %r{.*/+}. When the probe path
contains only backslashes (e.g. shared\rails\app\foo.rb sent by IDE tooling
on Windows), the loop's '/' check is false, so the stripping never fires.
With no stripping, only the full suffix is tested against registry paths;
the typical Linux deployment doesn't have the source-repo prefix
(shared/rails/) in its filesystem paths, so the match fails and the probe
stays at RECEIVED instead of progressing to INSTALLED.
Surfaced via the system test test_probe_status_log_line_with_windows_path
in DataDog/system-tests, which sends sourceFile with backslashes against a
Rails weblog whose runtime paths lack the shared/rails/ prefix.
Fix: normalize backslashes upfront in both functions before the stripping
loop, mirroring the pattern Utils.path_can_match_spec? already uses.
Spec coverage in spec/datadog/di/code_tracker_spec.rb covers:
- Windows-style probe path with prefix stripping required (the failing case)
- Windows-style + uppercase, exercising both backslash normalization and
the case-insensitive fallback
- Absolute Windows-style path
Verified: 3 new specs pass; full code_tracker_spec/utils_spec/instrumenter_spec/
probe_spec suite (203 examples) passes; standardrb clean; steep clean.
Co-Authored-By: Claude <[email protected]>
This reverts commit 25989b0.
Two follow-ups from review: 1. raise_if_probe_in_loaded_features now strips leading directory components from probe.file when searching $LOADED_FEATURES, matching the behavior of CodeTracker#iseqs_for_path_suffix. Without this, probes whose sourceFile carries a source-repo prefix that does not exist on disk (e.g. "shared\rails\app\foo.rb" when the runtime path is "/app/foo.rb") would fall through to the generic DITargetNotDefined error instead of the more specific DITargetNotInRegistry. Behavior of probe installation itself is unchanged — only the diagnostic error message becomes more precise. 2. sig/datadog/di/utils.rbs: add :: prefix to the stdlib classes on the two lines this PR modifies (normalize_windows_separators and path_matches_suffix?). path_can_match_spec? is left as-is since this PR did not touch its signature. Spec: spec/datadog/di/instrumenter_spec.rb gains a test that drives a backslash-prefixed probe path through hook_line and asserts the DITargetNotInRegistry path is reached. Verified: standardrb clean, steep clean, 204 examples pass across utils_spec / code_tracker_spec / instrumenter_spec / probe_spec. Co-Authored-By: Claude <[email protected]>
Replace the unconditional removal with prerelease-aware version gates, matching the pattern used in #6918 for DEBUG-4675 and the APMLP-1047 entries above. `<2.34.0-dev` (not `<2.34.0`) so the dev tracer is not covered by the bug marker — per semver prerelease ordering, `2.34.0-dev < 2.34.0` is true, so `<2.34.0` would skip the test on the tracer that contains the fix. Companion to DataDog/dd-trace-rb#5754 (merged), which makes `Utils.path_matches_suffix?` and `Utils.path_can_match_spec?` case-insensitive (DEBUG-5107) and tolerant of Windows-style backslash separators in user-supplied probe source paths (DEBUG-5111).
What does this PR do?
Makes
Datadog::DI::Utils.path_matches_suffix?andpath_can_match_spec?case-insensitive (DEBUG-5107) and tolerant of Windows-style backslash separators (DEBUG-5111) in the user-supplied probesourceFile.Concretely:
path_matches_suffix?gains an opt-incase_insensitive:keyword (defaultfalse) and unconditionally translates backslashes to forward slashes in the suffix. The case-sensitive default preserves uniqueness on case-sensitive deployments where two files differ only by case.CodeTracker#iseqs_for_path_suffix,CodeTracker#resolve_path_suffix,Utils.path_can_match_spec?,Instrumenter#raise_if_probe_in_loaded_features) run two passes: case-sensitive first, then case-insensitive as a fallback when no case-sensitive match exists. This mirrors the algorithm described in the design comment at the top ofutils.rb(steps 5–8).iseqs_for_path_suffix,resolve_path_suffix, andpath_can_match_spec?so the%r{.*/+}prefix-stripping regex fires on Windows-style probe paths (otherwise the loop's'/'check returns false and no stripping occurs).Utils.normalize_windows_separators.Motivation:
System tests
Test_Debugger_Line_Probe_Statuses::test_probe_status_log_line_with_different_casingand::test_probe_status_log_line_with_windows_pathare filed against Ruby asbug (DEBUG-5107)/bug (DEBUG-5111). Both tests deliver line probes withsourceFilemunged (uppercased / backslashes), simulating IDE tooling that does not normalize either. With the old matcher Ruby probes stayed at RECEIVED instead of moving to INSTALLED.Change log entry
Yes. Dynamic Instrumentation: Line probes now match
sourceFilecase-insensitively and accept Windows-style backslash separators, so probes delivered by IDE tooling that does not normalize the source path are now installed correctly.Additional Notes:
The previous comment in
path_matches_suffix?explicitly noted that "backslash is a legitimate character of a file name in Unix, therefore simply permitting forward or back slash is not sufficient." That concern applies to the actual on-disk path, not to the user-supplied probesourceFile. The change normalizes only the suffix/spec — the runtime path comes from Ruby'stp.path/$LOADED_FEATURES/ our code tracker and remains in its native form (onlydowncaseis applied for case-insensitive comparison, and only when the case-insensitive fallback pass is reached).Companion system-tests PR (draft) removing the bug markers: DataDog/system-tests#6920. Once #6920 merges and a system-tests refresh lands on master, the two debugger probe-status tests will run without the bug-marker xfail and verify this fix end-to-end on every weblog in the matrix.
How to test the change?
bundle exec rake test:di—spec/datadog/di/utils_spec.rbcovers case-sensitive default behavior, thecase_insensitive: trueparameter, backslash suffixes (basename and absolute), and case-mismatch cases.spec/datadog/di/code_tracker_spec.rbcovers the two-pass behavior iniseqs_for_path_suffix(correctly-cased unique match, case-insensitive fallback, ambiguous fallback raisingMultiplePathsMatch) and the Windows-backslash + prefix-stripping case that exercises the suffix-shortening loop after normalization.DEBUGGER_PROBES_STATUSagainst the rails72 and rails80 weblogs (the two weblog variants whose test apps implement the harness's/debugger/initendpoint) and both forced tests passed (7 passed, 5 skipped, 0 failed). The force-execute entry has since been reverted in 1aad557244; full system-test coverage across the rest of the matrix depends on [ruby] DEBUG-5107 / DEBUG-5111: Enable probe-status path-matching edge-case tests system-tests#6920 landing and a system-tests refresh reaching master.