APMSP-3550 DI: reject method probes on Datadog namespace#5907
Conversation
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.
Typing analysisNote: Ignored files are excluded from the next sections.
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 58cdfa5 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-06-24 17:16:44 Comparing candidate commit 58cdfa5 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.
|
|
@codex review |
There was a problem hiding this comment.
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_methodpre-check to forbid probes whosetype_nameis"Datadog"or starts with"Datadog::", raisingDatadog::DI::Error::ProbeTargetForbidden. - Introduced the new
ProbeTargetForbiddenerror class (and corresponding RBS signature). - Added RSpec coverage for allowed/forbidden
type_namevariants, 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.
There was a problem hiding this comment.
💡 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".
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]>
|
@codex review |
There was a problem hiding this comment.
💡 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".
|
I think there might be uses cases where DI on other non-DI parts of How about limiting this to |
|
@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`.
What does this PR do?
Rejects method probes whose target class or module is in the
Datadognamespace. A probe whose
type_nameequals"Datadog"or starts with"Datadog::"now raisesError::ProbeTargetForbiddenat hook timeinstead 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'sown code paths re-enter the probe wrapper while a snapshot is being
built. #5560 fixes this for stdlib methods that DI calls internally
(
String#lengthviaSecureRandom.uuid, etc.) by adding a fiber-localre-entrancy guard around the wrapper.
The Datadog namespace itself needs a stronger answer than the guard:
The classes are
@api privateand their shape changes betweenreleases.
Datadog::DI::Serializer#serialize_argswould eitherno-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.
ERRORstatus back tothe 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_methodchecksprobe.type_nameagainst"Datadog"and the
"Datadog::"prefix before doing anything else. The check istextual on the probe's declared type name and runs before class
resolution, so a probe like
Datadog::NotARealClass::AtAll#noopis rejected withProbeTargetForbiddenrather thanDITargetNotDefined— the user getsa 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 rejectionbranch 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_probevia the existingrescue => excbranch — it is recorded as a failed probe and anERRORstatus is sent to the backend without any further changes tothe manager or notifier.
How to test the change?
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 withProbeTargetForbidden'Datadog::NotARealClass::AtAll'— rejected before class resolution,proving the check runs first
'DatadogLike'— not in the Datadog namespace; falls through toDITargetNotDefinedas expected