Skip to content

DEBUG-5107 DEBUG-5111 DI: tolerant probe path matching#5754

Merged
p-datadog merged 7 commits into
masterfrom
fix-debug-5107-5111-path-matching
May 13, 2026
Merged

DEBUG-5107 DEBUG-5111 DI: tolerant probe path matching#5754
p-datadog merged 7 commits into
masterfrom
fix-debug-5107-5111-path-matching

Conversation

@p-datadog

@p-datadog p-datadog commented May 13, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Makes Datadog::DI::Utils.path_matches_suffix? and path_can_match_spec? case-insensitive (DEBUG-5107) and tolerant of Windows-style backslash separators (DEBUG-5111) in the user-supplied probe sourceFile.

Concretely:

  • path_matches_suffix? gains an opt-in case_insensitive: keyword (default false) 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.
  • Callers that resolve probe paths against a set of known paths (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 of utils.rb (steps 5–8).
  • Backslash normalization happens once up-front in the suffix-stripping loops of iseqs_for_path_suffix, resolve_path_suffix, and path_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).
  • The shared backslash-translation operation is extracted as Utils.normalize_windows_separators.

Motivation:

System tests Test_Debugger_Line_Probe_Statuses::test_probe_status_log_line_with_different_casing and ::test_probe_status_log_line_with_windows_path are filed against Ruby as bug (DEBUG-5107) / bug (DEBUG-5111). Both tests deliver line probes with sourceFile munged (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 sourceFile case-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 probe sourceFile. The change normalizes only the suffix/spec — the runtime path comes from Ruby's tp.path / $LOADED_FEATURES / our code tracker and remains in its native form (only downcase is 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:dispec/datadog/di/utils_spec.rb covers case-sensitive default behavior, the case_insensitive: true parameter, backslash suffixes (basename and absolute), and case-mismatch cases. spec/datadog/di/code_tracker_spec.rb covers the two-pass behavior in iseqs_for_path_suffix (correctly-cased unique match, case-insensitive fallback, ambiguous fallback raising MultiplePathsMatch) and the Windows-backslash + prefix-stripping case that exercises the suffix-shortening loop after normalization.
  • System tests: the fix was verified end-to-end while a temporary force-execute entry was on the branch — commit a4298f6a9b ran DEBUGGER_PROBES_STATUS against the rails72 and rails80 weblogs (the two weblog variants whose test apps implement the harness's /debugger/init endpoint) 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.

p-ddsign added 2 commits May 13, 2026 10:12
…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.
@p-datadog
p-datadog requested a review from a team as a code owner May 13, 2026 14:14
@p-datadog p-datadog added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label May 13, 2026
@dd-octo-sts dd-octo-sts Bot added the debugger Live Debugger (+Dynamic Instrumentation, +Symbol Database) label May 13, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread lib/datadog/di/utils.rb Outdated
Comment on lines +106 to +107
path = path.downcase
suffix = suffix.tr('\\', '/').downcase

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.rbpath_matches_suffix? only downcases when case_insensitive: true; path_can_match_spec? runs the suffix-shortening loop twice, case-sensitive first.
  • lib/datadog/di/code_tracker.rbiseqs_for_path_suffix and resolve_path_suffix each 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.find first, 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: true parameter; 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 paths covering unique match on correctly-cased lookup, fallback when no case-sensitive match exists, and MultiplePathsMatch when the fallback is ambiguous.

Comment thread .github/forced-tests-list.cfg Outdated
Comment on lines +16 to +17
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into this — the cleanup is captured in the PR description's "⚠️ Before merging" section. The forced-tests entries plus the 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.

@datadog-prod-us1-4

datadog-prod-us1-4 Bot commented May 13, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 97.16% (-0.00%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: bf3fc69 | Docs | Datadog PR Page | Give us feedback!

@Strech Strech left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one design suggestion

Comment thread lib/datadog/di/utils.rb Outdated
Comment on lines +132 to +134
# Normalize Windows-style backslash separators (DEBUG-5111) so the
# suffix-shortening loop's "/+" regex can strip leading components.
spec = spec.tr('\\', '/')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module is utils, does it make sense to have a method for windows path normalization instead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_separators module function; path_matches_suffix? and path_can_match_spec? use it.
  • sig/datadog/di/utils.rbs — added signature.

p-ddsign and others added 2 commits May 13, 2026 11:17
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]>
@pr-commenter

pr-commenter Bot commented May 13, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-05-13 19:09:02

Comparing candidate commit bf3fc69 in PR branch fix-debug-5107-5111-path-matching with baseline commit a848ac1 in branch master.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 44 metrics, 1 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:line instrumentation - untargeted

  • 🟥 throughput [-3809.987op/s; -3763.589op/s] or [-15.210%; -15.025%]

p-ddsign and others added 3 commits May 13, 2026 12:33
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]>
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]>
@p-datadog
p-datadog merged commit 0e70637 into master May 13, 2026
582 of 584 checks passed
@p-datadog
p-datadog deleted the fix-debug-5107-5111-path-matching branch May 13, 2026 19:31
@dd-octo-sts dd-octo-sts Bot added this to the 2.34.0 milestone May 13, 2026
p-datadog pushed a commit to DataDog/system-tests that referenced this pull request May 13, 2026
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).
@Strech Strech mentioned this pull request May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos debugger Live Debugger (+Dynamic Instrumentation, +Symbol Database)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants