Conversation
|
👋 Hey @DataDog/ruby-guild, please fill "Change log entry" section in the pull request description. If changes need to be present in CHANGELOG.md you can state it this way **Change log entry**
Yes. A brief summary to be placed into the CHANGELOG.md(possible answers Yes/Yep/Yeah) Or you can opt out like that **Change log entry**
None.(possible answers No/Nope/None) Visited at: 2026-08-20 22:24:44 UTC |
dd-octo-sts
Bot
added
the
debugger
Typing analysisNote: Ignored files are excluded from the next sections. Untyped methodsThis PR introduces 1 partially typed method, and clears 1 partially typed method. It increases the percentage of typed methods from 71.2% to 71.24% (+0.04%). Partially typed methods (+1-1)❌ Introduced:If you believe a method or an attribute is rightfully untyped or partially typed, you can add |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 3da71b6 | Docs | View more details | Give us feedback! |
3119c88 to
6983411
Compare
BenchmarksBenchmark execution time: 2026-09-23 19:24:40 Comparing candidate commit 3da71b6 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 51 metrics, 1 unstable metrics.
|
…erators Add a wall-time deadline check to Evaluator#filter, #all, #any (every EVALUATION_DEADLINE_CHECK_INTERVAL items) that raises a new DI::Error::EvaluationTimeout (subclass of ExpressionEvaluationError) when the per-invocation deadline is exceeded. The deadline lives on the per-invocation Context (Context#deadline_ns) to avoid racing the shared Evaluator instance across application threads. nil deadline preserves existing unbounded behavior.
…e eval Add the max_time_to_evaluate_ms setting (env DD_DYNAMIC_INSTRUMENTATION_EVALUATION_TIMEOUT_MS, default 50) and resolve a per-invocation deadline (Context#deadline_ns) before condition evaluation in both instrumenter hit paths and before template segment evaluation in ProbeNotificationBuilder. An EvaluationTimeout is surfaced through the existing condition-eval-failed callback as an evaluation-error snapshot with no captures, with a telemetry counter and debug log.
Evaluator cooperative-timeout tests (filter/all/any with deadline in the past, no deadline, far-future deadline, and deadline crossed midway via a stubbed clock). Instrumenter condition-timeout test asserting an over-budget condition produces an evaluation-error snapshot with no captures via the existing callback, plus an evaluation_timeouts telemetry counter. Builder settings double defaults max_time_to_evaluate_ms to nil.
4e26409 to
6f4c5d5
Compare
p-datadog
changed the title
p-datadog
added
the
AI Generated
p-datadog
changed the title
…d configurations The DD_DYNAMIC_INSTRUMENTATION_EVALUATION_TIMEOUT_MS env var, declared in lib/datadog/di/configuration.rb (option max_time_to_evaluate_ms), was not registered in supported-configurations.json. config_helper.rb rejects any o.env var missing from the registry, so every spec loading DI configuration raised RuntimeError: Missing DD_DYNAMIC_INSTRUMENTATION_EVALUATION_TIMEOUT_MS env/configuration in "supported-configurations.json" file This produced 78 spec failures per Ruby version in every standard test batch and failed all 18 test jobs (Ruby 2.5..4.0 standard, Nix x86_64/aarch64, macOS 3.0..4.0), plus a lint failure (CustomCops/EnvStringValidationCop at configuration.rb:157). Add the entry to supported-configurations.json matching the existing DD_DYNAMIC_INSTRUMENTATION_MAX_TIME_TO_SERIALIZE format (version A, type int, default 50, the option's declared default) and regenerate lib/datadog/core/configuration/supported_configurations.rb via `rake local_config_map:generate`. Verified: DI configuration loads without the Missing-env error and spec/datadog/di/el/evaluator_spec.rb passes (17 examples, 0 failures).
evaluation_deadline_exceeded? is declared () -> bool but its body
`deadline && (...)` returns nil when no deadline is set (nil | bool),
which Steep rejects as a method-body type mismatch
(Ruby::MethodBodyTypeMismatch). Use `!deadline.nil? && (...)` so the
method returns false (not nil) when no deadline is set, matching the
declared bool signature and the existing docstring ("Returns false when
no deadline is set"). Behavior is unchanged: nil and false are both
falsy at the call sites (`if ... && evaluation_deadline_exceeded?`).
Verified: `bundle exec steep check` reports no type errors.
StandardRB flagged two offenses introduced by this PR's spec additions: - spec/datadog/di/el/evaluator_spec.rb:237: Style/TernaryParentheses (parenthesize the complex ternary condition) - spec/datadog/di/instrumenter_spec.rb:2137: Layout/EmptyLines (extra blank line) Both are auto-corrected by `rake standard:fix` with no semantic change to the specs. Verified: `bundle exec rake standard` reports no offenses.
The PR added the max_time_to_evaluate_ms setting (read by ProbeNotificationBuilder#evaluation_deadline_ns during template segment evaluation) but the "di settings" double in probe_notification_builder_spec.rb did not stub it. With strict doubles, the builder's settings.dynamic_instrumentation .max_time_to_evaluate_ms call raised #<Double "di settings"> received unexpected message :max_time_to_evaluate_ms with (no args) failing 4 template-segment specs per Ruby version (8 test jobs). Stub :max_time_to_evaluate_ms to return nil, matching the production sentinel (evaluation_deadline_ns does `return nil unless budget_ms`), so template evaluation stays unbounded and the existing template-building specs are unaffected. This mirrors the instrumenter_spec.rb stub added by this PR's test commit. Verified: spec/datadog/di/integration/probe_notification_builder_spec.rb passes (6 examples, 0 failures).
* origin/master: (28 commits) Switch a few more `RARRAY_AREF` with `rb_ary_entry` + enforce entries are threads Get rid of hanging parens Wording tweaks after PR review [🤖] Update images-rb pin: https://github.com/DataDog/dd-trace-rb/actions/runs/35854730927 Add changelog Avoid potential overflow in conversion if `value` is an `int` Move reading CPU-time inside `update_metrics_and_sample` [NO-TICKET] Profiling: Fix "CPU time going backwards between samples" due to GC Re-enable appsec:rack rack-latest on Ruby 2.5 now that Rack 3.2.7 is released Re-enable rack-latest on Ruby 2.5 now that Rack 3.2.7 is released changelog: fix empty http.route tag on 405s for grape 2.3-3.0 [🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/35773936776 fix(grape): fall back to pattern.path for the route tag test(grape): normalize appraise version order to 1, 2, 3, latest chore(ci): track grape in the edge:update allowlist [🤖] Update datadog gem version to 2.44.0.dev [🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/35717772810 test(grape): pin rack 2 for grape 1, disable broken gemsets temporarily Bump to version 2.43.0 [🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/35715728853 ...
…heck, dedup helper Represent the evaluation deadline as float seconds (CLOCK_MONOTONIC :float_second) throughout the eval-timeout path, per the project convention that DI time intervals are held in seconds. Context#deadline_ns becomes Context#deadline (Float), and the Evaluator reads :float_second instead of :nanosecond. This matches the snapshot-capture-timeout impl (#6205) and the design docs. Add a between-segment deadline check in ProbeNotificationBuilder#evaluate_template that raises EvaluationTimeout before an EL::Expression template segment when the budget is exhausted, bounding pathological template segments. The check is gated on EL::Expression segments so the per-segment rescue handler's segment.dsl_expr assumption holds. Deduplicate the evaluation-deadline resolver: Evaluator.evaluation_deadline (class) and Evaluator.evaluation_deadline_exceeded?(context) (class) are now the single sources; Instrumenter and ProbeNotificationBuilder call them instead of carrying private copies. Reject negative max_time_to_evaluate_ms with ArgumentError at the setting boundary (zero is accepted as an exhausted-budget sentinel), matching max_time_to_serialize_ms validation. Fix the method-probe condition-error callback call to pass the condition expression (3-arg), matching the ProbeManager callback signature and the line-probe path; previously the 2-arg call raised ArgumentError rescued silently, so a method-probe condition timeout never emitted. Reference DI::TELEMETRY_NAMESPACE (added to the DI module, matching #6205) in the evaluation_timeouts telemetry counter. Expand the GettingStarted rows and add a changelog fragment. RBS updated. Steep/standard deferred to CI (no local steep/standard gemfile).
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved timeout coverage, responder compatibility, and template evaluation findings remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Adds configurable wall-time budgets for Dynamic Instrumentation condition and template evaluation.
Changes:
- Adds timeout configuration, environment support, validation, and documentation.
- Implements cooperative evaluation deadlines, timeout errors, and telemetry.
- Updates tests and RBS signatures.
| File | Description |
|---|---|
unreleased/20260921180003.json |
Adds changelog entry. |
supported-configurations.json |
Registers environment configuration. |
spec/datadog/di/spec_helper.rb |
Updates test defaults. |
spec/datadog/di/probe_notification_builder_spec.rb |
Tests template timeouts. |
spec/datadog/di/integration/probe_notification_builder_spec.rb |
Updates integration settings. |
spec/datadog/di/instrumenter_spec.rb |
Tests condition timeouts. |
spec/datadog/di/el/evaluator_spec.rb |
Tests evaluator deadlines. |
spec/datadog/di/configuration/settings_spec.rb |
Tests setting validation. |
sig/datadog/di/instrumenter.rbs |
Updates instrumenter signatures. |
sig/datadog/di/error.rbs |
Declares timeout error type. |
sig/datadog/di/el/evaluator.rbs |
Declares evaluator APIs. |
sig/datadog/di/context.rbs |
Declares context deadline APIs. |
sig/datadog/di.rbs |
Declares telemetry namespace. |
sig/datadog/core/configuration/settings.rbs |
Declares setting signatures. |
lib/datadog/di/probe_notification_builder.rb |
Applies template deadlines. |
lib/datadog/di/instrumenter.rb |
Applies condition deadlines. |
lib/datadog/di/error.rb |
Defines timeout error. |
lib/datadog/di/el/evaluator.rb |
Implements deadline checks. |
lib/datadog/di/context.rb |
Stores evaluation deadlines. |
lib/datadog/di/configuration.rb |
Adds timeout configuration. |
lib/datadog/di.rb |
Adds telemetry namespace. |
lib/datadog/core/configuration/supported_configurations.rb |
Registers supported configuration. |
docs/GettingStarted.md |
Documents the new setting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # the probe notifier builder requires a context. | ||
| begin | ||
| responder.probe_condition_evaluation_failed_callback(context, exc) | ||
| responder.probe_condition_evaluation_failed_callback(context, condition, exc) |
| if segment.is_a?(EL::Expression) && Datadog::DI::EL::Evaluator.evaluation_deadline_exceeded?(context) | ||
| raise DI::Error::EvaluationTimeout, "expression evaluation timeout" |
| context.deadline = Datadog::DI::EL::Evaluator.evaluation_deadline(settings) | ||
| evaluation_errors = [] | ||
| message = template_segments.map do |segment| | ||
| if segment.is_a?(EL::Expression) && Datadog::DI::EL::Evaluator.evaluation_deadline_exceeded?(context) | ||
| raise DI::Error::EvaluationTimeout, "expression evaluation timeout" | ||
| end |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: 3da71b6ba1
ℹ️ 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".
| locals: serializer.combine_args(args, kwargs, target_self), | ||
| target_self: target_self, | ||
| probe: probe, settings: settings, serializer: serializer, | ||
| deadline: Datadog::DI::EL::Evaluator.evaluation_deadline(settings), |
There was a problem hiding this comment.
When a condition does not invoke all, any, or filter, this deadline is never read: for example, with max_time_to_evaluate_ms = 0, a ref/comparison condition still evaluates and emits its normal snapshot, while a catastrophic matches condition can run until the separate 500 ms regexp timeout despite the default 50 ms budget. Check the deadline at the condition boundary and ensure expensive non-collection operators honor the remaining budget so the advertised guardrail applies to all conditions.
Useful? React with 👍 / 👎.
| # the probe notifier builder requires a context. | ||
| begin | ||
| responder.probe_condition_evaluation_failed_callback(context, exc) | ||
| responder.probe_condition_evaluation_failed_callback(context, condition, exc) |
There was a problem hiding this comment.
When a method-probe condition raises, this now invokes the callback with three arguments, but Datadog::DI::ProcResponder#probe_condition_evaluation_failed_callback still accepts only (context, exc). Method probes using this repository's proc adapter therefore raise an ArgumentError inside the nested rescue and never call their configured failure proc; update the adapter and its failure-proc forwarding to accept the expression argument.
Useful? React with 👍 / 👎.
| if segment.is_a?(EL::Expression) && Datadog::DI::EL::Evaluator.evaluation_deadline_exceeded?(context) | ||
| raise DI::Error::EvaluationTimeout, "expression evaluation timeout" |
There was a problem hiding this comment.
For a log probe with capture_snapshot: true, captures are serialized before evaluate_template is called, and this timeout is rescued into an evaluation-error message without clearing those captures. The resulting snapshot therefore still contains locals/arguments even though the newly documented behavior says an over-budget template evaluation produces no captured snapshot; either evaluate the template before capture or discard the already-built captures when an EvaluationTimeout is recorded.
Useful? React with 👍 / 👎.
| # evaluation unbounded (preserves existing behavior for callers | ||
| # that do not supply a deadline). | ||
| def evaluation_deadline_exceeded? | ||
| self.class.evaluation_deadline_exceeded?(@context) |
There was a problem hiding this comment.
When the same probe is evaluated concurrently, its Expression reuses one evaluator and each call overwrites that evaluator's @context; this helper then rereads the shared @context at every collection interval. A second invocation can therefore replace the first invocation's deadline, allowing the first to run past its own budget, or make the second observe an older deadline and time out prematurely. Capture the invocation context or deadline in evaluation-local state rather than consulting the shared evaluator field during iteration.
Useful? React with 👍 / 👎.
| block.call([key, value], key, value) | ||
| end.to_h | ||
| i = 0 | ||
| result = {} |
There was a problem hiding this comment.
When collection is a hash using compare_by_identity, constructing the filtered result as a normal {} changes its key semantics. Distinct key objects that are eql?—for example, two different strings with the same contents—can coexist in the input and in the previous Hash#select result, but assignments here collapse them into one entry, so subsequent len or index operations evaluate differently. Initialize the result with the source hash's identity mode preserved.
Useful? React with 👍 / 👎.
What does this PR do?
Adds a configurable wall-time budget for evaluating a Dynamic Instrumentation probe condition (
when) or message-template segment. The budget is controlled by themax_time_to_evaluate_mssetting (environment variableDD_DYNAMIC_INSTRUMENTATION_EVALUATION_TIMEOUT_MS, default 50 ms).Motivation:
Ensuring DI work is time bounded.
Change log entry
Yes. Dynamic Instrumentation: add a configurable wall-time budget for probe condition and message-template evaluation.
How to test the change?
Test_Debugger_Evaluation_Timeout_Line_Probe(DataDog/system-tests#7287)