Skip to content

APMSP-3550 DI: reject method probes on Datadog namespace#5907

Merged
p-datadog merged 6 commits into
masterfrom
di-reject-datadog-namespace-probes
Jun 25, 2026
Merged

APMSP-3550 DI: reject method probes on Datadog namespace#5907
p-datadog merged 6 commits into
masterfrom
di-reject-datadog-namespace-probes

Conversation

@p-datadog

Copy link
Copy Markdown
Member

What does this PR do?

Rejects method probes whose target class or module is in the Datadog
namespace. A probe whose type_name equals "Datadog" or starts with
"Datadog::" now raises Error::ProbeTargetForbidden at hook time
instead of being installed.

Companion to #5560.

Motivation

A method probe on a class the tracer itself uses (e.g. anything under
Datadog::Tracing, Datadog::DI, Datadog::Core::Telemetry) lets DI's
own code paths re-enter the probe wrapper while a snapshot is being
built. #5560 fixes this for stdlib methods that DI calls internally
(String#length via SecureRandom.uuid, etc.) by adding a fiber-local
re-entrancy guard around the wrapper.

The Datadog namespace itself needs a stronger answer than the guard:

  • There is no legitimate customer use case for probing tracer internals.
    The classes are @api private and their shape changes between
    releases.
  • A probe on, say, Datadog::DI::Serializer#serialize_args would either
    no-op every invocation (because the guard is held during snapshot
    building) or, if the guard were released, recurse without bound. Both
    outcomes are worse than refusing to install the probe.
  • Rejecting at hook time surfaces an immediate ERROR status back to
    the backend, so the user sees the problem in the Live Debugger UI
    rather than wondering why their probe never fires.

This is orthogonal to #5560: that PR makes stdlib probes safe; this PR
makes Datadog-namespace probes a refusal. Either can land first.

Change log entry

Yes. Dynamic Instrumentation: reject method probes that target classes
or modules in the Datadog namespace.

Implementation

Instrumenter#hook_method checks probe.type_name against "Datadog"
and the "Datadog::" prefix before doing anything else. The check is
textual on the probe's declared type name and runs before class
resolution, so a probe like
Datadog::NotARealClass::AtAll#noop is rejected with
ProbeTargetForbidden rather than DITargetNotDefined — the user gets
a precise reason. A name that happens to start with the letters
"Datadog" but does not have :: after it (e.g. "DatadogLike")
is not in the Datadog namespace and falls through to the normal
class-resolution path.

Line probes are unaffected: they install via TracePoint (self-disabling
during its callback) and do not carry a type_name, so the rejection
branch is unreachable for them. The rejection lives in hook_method,
not in the higher-level hook, for that reason.

The error flows through ProbeManager#add_probe via the existing
rescue => exc branch — it is recorded as a failed probe and an
ERROR status is sent to the backend without any further changes to
the manager or notifier.

How to test the change?

bundle exec rspec spec/datadog/di/instrumenter_spec.rb

87 examples, 0 failures, 8 pending (same 8 Ruby-2-only pending tests as
on master).

The new specs live under describe '.hook_method'
context 'when targeting a class in the Datadog namespace' and cover:

  • 'Datadog', 'Datadog::Tracing::SpanOperation',
    'Datadog::DI::Instrumenter' — all rejected with
    ProbeTargetForbidden
  • 'Datadog::NotARealClass::AtAll' — rejected before class resolution,
    proving the check runs first
  • 'DatadogLike' — not in the Datadog namespace; falls through to
    DITargetNotDefined as expected

Method probes that target a class or module under the Datadog namespace
let the tracer's own code paths re-enter the probe wrapper. The
re-entrancy guard added in #5560 prevents this for stdlib methods used
internally by DI; this commit takes the orthogonal approach for Datadog
namespace targets by rejecting them up front in hook_method.

A probe whose type_name equals "Datadog" or starts with "Datadog::"
now raises Error::ProbeTargetForbidden at hook time. The check is
textual on the probe's declared type name and runs before class
resolution, so non-existent Datadog-namespaced names are also rejected
with the new error rather than DITargetNotDefined. Line probes are
unaffected: they install via TracePoint, which is self-disabling during
its callback, and do not carry a type_name.

The error flows through ProbeManager#add_probe via the existing rescue
=> exc branch, so it is recorded as a failed probe and reported to the
backend as ERROR status without further changes.
@dd-octo-sts dd-octo-sts Bot added the debugger Live Debugger (+Dynamic Instrumentation, +Symbol Database) label Jun 16, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Typing analysis

Note: Ignored files are excluded from the next sections.

steep:ignore comments

This PR introduces 5 steep:ignore comments, and clears 5 steep:ignore comments.

steep:ignore comments (+5-5)Introduced:
lib/datadog/di/instrumenter.rb:121
lib/datadog/di/instrumenter.rb:135
lib/datadog/di/instrumenter.rb:358
lib/datadog/di/instrumenter.rb:360
lib/datadog/di/instrumenter.rb:683
Cleared:
lib/datadog/di/instrumenter.rb:109
lib/datadog/di/instrumenter.rb:123
lib/datadog/di/instrumenter.rb:346
lib/datadog/di/instrumenter.rb:348
lib/datadog/di/instrumenter.rb:671

Untyped methods

This PR introduces 7 partially typed methods, and clears 7 partially typed methods. It increases the percentage of typed methods from 65.52% to 65.54% (+0.02%).

Partially typed methods (+7-7)Introduced:
sig/datadog/di/instrumenter.rbs:38
└── def hook_method: (Probe probe, untyped responder) -> void
sig/datadog/di/instrumenter.rbs:41
└── def hook_line: (Probe probe, untyped responder) -> bool?
sig/datadog/di/instrumenter.rbs:45
└── def hook: (Probe probe, untyped responder) -> void
sig/datadog/di/instrumenter.rbs:49
└── def self.get_local_variables: (TracePoint trace_point) -> Hash[Symbol, untyped]
sig/datadog/di/instrumenter.rbs:50
└── def self.get_instance_variables: (Object self) -> Hash[Symbol, untyped]
sig/datadog/di/instrumenter.rbs:56
└── def line_trace_point_callback: (Probe probe, RubyVM::InstructionSequence? iseq, untyped responder, TracePoint tp) -> void
sig/datadog/di/instrumenter.rbs:60
└── def check_and_disable_if_exceeded: (Probe probe, untyped responder, Float di_start_time, ?Float accumulated_duration) -> void
Cleared:
sig/datadog/di/instrumenter.rbs:36
└── def hook_method: (Probe probe, untyped responder) -> void
sig/datadog/di/instrumenter.rbs:39
└── def hook_line: (Probe probe, untyped responder) -> bool?
sig/datadog/di/instrumenter.rbs:43
└── def hook: (Probe probe, untyped responder) -> void
sig/datadog/di/instrumenter.rbs:47
└── def self.get_local_variables: (TracePoint trace_point) -> Hash[Symbol, untyped]
sig/datadog/di/instrumenter.rbs:48
└── def self.get_instance_variables: (Object self) -> Hash[Symbol, untyped]
sig/datadog/di/instrumenter.rbs:54
└── def line_trace_point_callback: (Probe probe, RubyVM::InstructionSequence? iseq, untyped responder, TracePoint tp) -> void
sig/datadog/di/instrumenter.rbs:58
└── def check_and_disable_if_exceeded: (Probe probe, untyped responder, Float di_start_time, ?Float accumulated_duration) -> void

If you believe a method or an attribute is rightfully untyped or partially typed, you can add # untyped:accept on the line before the definition to remove it from the stats.

@p-datadog p-datadog changed the title DI: reject method probes on Datadog namespace APMSP-3550 DI: reject method probes on Datadog namespace Jun 16, 2026
@datadog-prod-us1-3

datadog-prod-us1-3 Bot commented Jun 16, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 50.00%
Overall Coverage: 90.02% (+0.00%)

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

@pr-commenter

pr-commenter Bot commented Jun 16, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-06-24 17:16:44

Comparing candidate commit 58cdfa5 in PR branch di-reject-datadog-namespace-probes with baseline commit 49368c6 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 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 ----------------------------------'

@p-datadog
p-datadog requested review from Copilot June 16, 2026 16:07
@p-datadog

Copy link
Copy Markdown
Member Author

@codex review

Copilot AI 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.

Pull request overview

Rejects Dynamic Instrumentation method probes that target classes/modules in the Datadog namespace, preventing probes from instrumenting tracer internals and avoiding re-entrancy/recursion hazards during snapshot processing.

Changes:

  • Added a hook_method pre-check to forbid probes whose type_name is "Datadog" or starts with "Datadog::", raising Datadog::DI::Error::ProbeTargetForbidden.
  • Introduced the new ProbeTargetForbidden error class (and corresponding RBS signature).
  • Added RSpec coverage for allowed/forbidden type_name variants, including the “reject before constant resolution” behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
spec/datadog/di/instrumenter_spec.rb Adds specs asserting Datadog-namespace method probes are rejected with ProbeTargetForbidden and that non-namespace lookalikes fall through normally.
lib/datadog/di/instrumenter.rb Implements the Datadog-namespace rejection in hook_method plus a helper datadog_namespace_type_name?.
lib/datadog/di/error.rb Adds Error::ProbeTargetForbidden for the new refusal behavior.
sig/datadog/di/instrumenter.rbs Updates RBS to include the new private helper signature.
sig/datadog/di/error.rbs Updates RBS to include ProbeTargetForbidden.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@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: 0478f87e47

ℹ️ 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/instrumenter.rb Outdated
p-ddsign and others added 4 commits June 16, 2026 12:27
Address review feedback on #5907:

- Strip Ruby's root-namespace prefix ("::") before the textual check,
  so a probe targeting "::Datadog::Tracing::SpanOperation" is rejected
  with ProbeTargetForbidden — same as the bare form. Without this,
  a "::"-prefixed name slipped past the check and fell through to
  Object.const_get, which raises NameError (rejected as
  DITargetNotDefined instead of ProbeTargetForbidden).

- Add tests for "::Datadog", "::Datadog::Tracing::SpanOperation",
  and "::DatadogLike" to cover both the rejection and the
  false-positive case under the normalization.

- Qualify the new RBS signature with the "::" root-namespace prefix
  for String, per the repo-wide RBS rule.

Verified: bundle exec rspec spec/datadog/di/instrumenter_spec.rb
(90 examples, 0 failures, 8 pending — same 8 Ruby-2-only pending
tests as before). bundle exec steep check lib/datadog/di/instrumenter.rb
clean.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
Replace the sub-and-compare implementation with a single anchored
regexp match: /\A(?:::)?Datadog(?:::|\z)/.

Benchmarks on a pinned, frequency-locked core (cpu9 @ 1200 MHz, no YJIT)
across an 8-input mix:

  C regexp                     354k i/s  (±0.8%)
  B multi-arg start_with?      227k i/s  (±3.3%)
  A current (sub + eq + start) 104k i/s  (±52.4%)

The current implementation allocates a string on every call (sub returns
a new String regardless of whether the prefix is present), which (a)
makes it the slowest of the three and (b) introduces large GC-driven
variance — ±52% even after pinning + freq lock removed CPU-side noise.
The regexp does no allocation and folds the two prefix alternatives into
one anchored state machine.

All 90 existing spec examples pass. RBS gains a constant declaration for
DATADOG_NAMESPACE_TYPE_NAME so Steep can resolve it.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
Address review feedback on #5907:

- Remove the `return false if cls_name.nil?` guard from
  `Instrumenter#datadog_namespace_type_name?`. The guard was needed
  when the predicate accepted `::String?`, but the only caller
  (`hook_method`) is invoked only for method probes, and the `Probe`
  constructor enforces that whenever `method_name` is set `type_name`
  must also be set. The nil branch was therefore unreachable in
  practice and untested.

- Tighten the RBS signature to `(::String cls_name) -> bool`.

- Narrow `probe.type_name` to a local at the `hook_method` call site
  with a `type_name && …` guard so Steep can see the non-nil flow.
  Reuse the local in the error message instead of reading
  `probe.type_name` twice.

- Fix two stale test comments that described the previous
  sub-and-compare implementation ("normalizes it away" / "stripped
  before the comparison") — the current implementation uses a regex
  with an optional `(?:::)?` prefix and a `(?:::|\z)` trailing
  boundary. Replace with comments that match the current behavior.

Verified: bundle exec rspec spec/datadog/di/instrumenter_spec.rb
(90 examples, 0 failures, 8 pending — same baseline as before).
bundle exec rake typecheck passes.

Co-Authored-By: Claude <[email protected]>
@p-datadog
p-datadog marked this pull request as ready for review June 23, 2026 20:55
@p-datadog
p-datadog requested a review from a team as a code owner June 23, 2026 20:55
@p-datadog

Copy link
Copy Markdown
Member Author

@codex review

@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: 9aa877e2c7

ℹ️ 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/instrumenter.rb
@eregon

eregon commented Jun 24, 2026

Copy link
Copy Markdown
Member

I think there might be uses cases where DI on other non-DI parts of Datadog:: would be useful (e.g. if a customer wants to find out if some method under Datadog is called or not, e.g. to investigate some bug).

How about limiting this to Datadog::DI (and Datadog::Tracing and Datadog::Core::Telemetry if needed)?

@p-datadog

p-datadog commented Jun 24, 2026

Copy link
Copy Markdown
Member Author

@eregon Yes that is a possibility. I will be happy to consider it if there is a customer request to set probes on Datadog code. For now, since DI uses Core (obviously) as well as integrates with Tracing, we should prohibit all targeting of Datadog namespace to guarantee runtime safety of DI.

The textual rejection in hook_method missed alias forms that
Object.const_get still resolves to the same top-level constants. A method
probe with type_name "Object::Datadog::DI::Instrumenter" did not match
/\A(?:::)?Datadog(?:::|\z)/, but symbolize_class_name resolved it to the
real Datadog::DI::Instrumenter and prepended onto tracer internals —
exactly the re-entrancy/recursion hazard the rejection prevents. Top-level
constants are constants of Object, so any number of leading "Object::"
segments (with an optional leading "::") reaches the top-level Datadog.

Extend the regex to /\A(?:::)?(?:Object::)*Datadog(?:::|\z)/. "Object::" is
the only alias path: "Foo::Datadog" does not resolve through const_get for
an arbitrary Foo, and "Kernel::Datadog" raises NameError. The check stays
textual and pre-resolution, preserving the existing behavior that an
unknown Datadog-namespaced class raises ProbeTargetForbidden (not
DITargetNotDefined).

Specs add positive cases (Object::Datadog, Object::Datadog::DI::Instrumenter,
::Object::Datadog::Tracing::SpanOperation, Object::Object::Datadog) and a
negative case (Object::DatadogLike falls through to DITargetNotDefined).

Addresses codex review comment on PR #5907.

Verified: regex behavior checked directly against the full alias matrix via
ruby -e (all cases pass). instrumenter_spec.rb NOT executed: `bundle install`
fails in this environment (cannot resolve rake — no network access), so
`bundle exec rspec` cannot run. Spec syntax checked with `ruby -c`.
@p-datadog
p-datadog merged commit 05d39cb into master Jun 25, 2026
587 checks passed
@p-datadog
p-datadog deleted the di-reject-datadog-namespace-probes branch June 25, 2026 13:08
@dd-octo-sts dd-octo-sts Bot added this to the 2.37.0 milestone Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debugger Live Debugger (+Dynamic Instrumentation, +Symbol Database)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants