Conversation
|
Thank you for updating Change log entry section 👏 Visited at: 2025-12-05 17:39:23 UTC |
BenchmarksBenchmark execution time: 2026-03-27 16:34:19 Comparing candidate commit 6d63f39 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 0 unstable metrics.
|
feef350 to
8be2f98
Compare
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 3539fa7 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
I think this is the right decision -- managing multiple extensions adds complexity AND makes installation slower. |
ivoanjo
left a comment
There was a problem hiding this comment.
This is cool! Left a few notes :)
Address review comment: headers not needed when no C APIs are exposed. - Deleted ext/libdatadog_api/ddsketch.h - Deleted ext/libdatadog_api/di.h - Added forward declarations in ext/libdatadog_api/init.c Co-Authored-By: Claude <noreply@anthropic.com>
Address review comments: reduce file explosion by inlining helpers. - Moved ddtrace_imemo_type to datadog_ruby_common.h as static inline (comment 3) - Moved Ruby internal prototypes into di.c (comment 4) - Moved ddtrace_imemo_iseq_p into di.c as static (comment 5) - Simplified ddtrace_imemo_iseq_p to single return statement (comment 6) - Removed duplicate DDTRACE_UNUSED define, using datadog_ruby_common.h (comment 7) - Deleted ruby_helpers.c, ruby_helpers.h, ruby_internal.h Co-Authored-By: Claude <noreply@anthropic.com>
Address review comments: add ruby/debug reference, trim C comment, move iseq type documentation to where it's consumed, remove struct wrapper and unnecessary RB_GC_GUARD. - Added reference to ruby/debug iseq_collector.c (comment 8) - Moved iseq type analysis docs to lib/datadog/di.rb (comment 9) - Removed struct wrapper, pass VALUE* directly (comment 10) - Removed RB_GC_GUARD (comment 10) Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for confirming — glad the approach makes sense! |
- Removed trailing whitespace on blank line in ext/libdatadog_api/di.c:75 - Added @return YARD tag to file_iseqs in lib/datadog/di.rb Co-Authored-By: Claude <noreply@anthropic.com>
Address review comments: the exception class name is already reported via the :type field, so returning msg.class.name in :message was redundant. Return "<REDACTED: not a string value>" instead. Also updated the comment to note that Class#name can technically be overridden by customers (though unlikely). - Fixed in lib/datadog/di/probe_notification_builder.rb:187-190 (code) - Fixed in lib/datadog/di/probe_notification_builder.rb:169-171 (comment) - Fixed in spec/datadog/di/probe_notification_builder_spec.rb:574-576 (test) Co-Authored-By: Claude <noreply@anthropic.com>
Address review comment: the section heading and framing were too arcane. Renamed to "What Data Is Captured" with clearer structure explaining what each snapshot includes. - Fixed in docs/DynamicInstrumentation.md:293-312 Co-Authored-By: Claude <noreply@anthropic.com>
The web UI expects throwable to have {type, message, stacktrace[]},
matching Java and Python tracers. Without stacktrace, the UI crashes
with "Could not load snapshot" (TypeError on .map() of undefined).
Parses exception.backtrace strings into {fileName, function, lineNumber}
frame objects. Returns nil when backtrace is absent (unraised exceptions).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The charted spec requires stacktrace to always be an array. An absent backtrace now produces [] instead of nil. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The DI test suite (spec:di:di_with_ext) was never run in CI because it was missing from the Matrixfile. This meant the C extension tests (all_iseqs, exception_message, iseq_type) and integration tests that depend on compiled native code were never executed. Also fix the throwable integration test to assert stacktrace array, which was added by the serialize_throwable change but the test was never updated because CI never ran it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This reverts commit 12ea888.
DI tests were never run in CI because the task was missing from the Matrixfile. spec:main explicitly excludes spec/datadog/di/ and di_with_ext is the only task that covers them, but it was not listed in the Matrixfile that CI uses to generate test batches. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The throwable now includes a stacktrace array (from the C extension commit). Also update error message assertion for the new raise_if_probe_in_loaded_features format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The cherry-picked commit from di-per-method-iseq also changed the DITargetNotInRegistry error message pattern, which doesn't apply on this branch. Restore the original pattern. Co-Authored-By: Claude <noreply@anthropic.com>
The di_with_ext RSpec task is defined inside `namespace :di` (Rakefile:440), making the full task name `spec:di:di_with_ext`. Both the Matrixfile and `spec:all` referenced it as `spec:di_with_ext` (without the `di:` namespace prefix), causing "Don't know how to build task 'spec:di_with_ext'" errors in every CI batch that includes this task. Fix: use `di:di_with_ext` in both Matrixfile and spec:all, matching the pattern used by other namespaced tasks like `profiling:main`. Co-Authored-By: Claude <noreply@anthropic.com>
The di_with_ext glob pattern `spec/datadog/di/**/*_spec.rb` also
matches `spec/datadog/di/contrib/{active_record,rails}/**/*_spec.rb`.
Those contrib specs require Rails (`require 'rails/all'`) which is
not available in the base Gemfile used by di_with_ext, causing
"LoadError: cannot load such file -- rails/all" and
"0 examples, 0 failures, 2 errors occurred outside of examples".
The DI contrib specs have their own dedicated rake tasks
(spec:di:active_record, spec:di:rails) that run with Rails gemfiles.
Co-Authored-By: Claude <noreply@anthropic.com>
StandardRB requires double-quoted symbols for consistency. Change :'di:di_with_ext' to :"di:di_with_ext" to match the pattern used elsewhere in the Rakefile (e.g., :"profiling:all"). Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex Review for bugs and edge-cases |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6577c28e9
ℹ️ 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 comment: extract regex into a constant for readability. - Added BACKTRACE_FRAME_PATTERN constant in lib/datadog/di/probe_notification_builder.rb - Used constant in format_backtrace method Co-Authored-By: Claude <noreply@anthropic.com>
…tion The BACKTRACE_FRAME_PATTERN constant was added to lib/datadog/di/probe_notification_builder.rb but its type declaration was not added to the corresponding RBS signature file, causing Ruby::UnknownConstant errors in Steep. Not verified locally (steep not available in this environment). Co-Authored-By: Claude <noreply@anthropic.com>
What does this PR do?
Adds two C extension functions to libdatadog_api for Dynamic Instrumentation:
all_iseqs: returnsRubyVM::InstructionSequenceobjects for loaded code, needed for instrumenting third-party code with line probes. Not yet wired into instrumentation — that happens in DI: backfill CodeTracker registry with iseqs for pre-loaded files #5496.exception_message: returns the raw argument passed to the Exception constructor, bypassing any Ruby-levelmessagemethod overrides.Integrates
exception_messageintoProbeNotificationBuilderso method probe snapshots now populate thethrowablefield (type, message, stacktrace).The C extension is now a hard requirement for DI —
environment_supported?checks for it, and the new code calls it directly with no fallback.DI specs under
spec/datadog/di/are moved fromspec:mainto the newspec:di:di_with_exttask, which compiles the extension before running tests. The task is added to the Matrixfile so CI runs them.Motivation:
exception_messageaccesses the raw C-level message, bypassing overriddenmessagemethods.Change log entry
Yes. Dynamic Instrumentation: exception messages are now safely reported in method probe snapshots. The libdatadog_api C extension is now required for Dynamic Instrumentation.
Additional Notes:
The C code lives in the existing libdatadog_api extension (does not use libdatadog itself — this avoids creating another extension).
An error boundary is added around
probe_executed_callbackin method probes so DI errors never propagate to or replace customer exceptions.Non-string exception constructor arguments are reported as
<REDACTED: not a string value>rather than calling.to_s(which could invoke customer code). Nil arguments pass through as nil.Follow-up work:
all_iseqs/file_iseqsto enable line probes on code loaded before DI starts (DI: backfill CodeTracker registry with iseqs for pre-loaded files #5496)trace_pointsfield of iseqs to detect line probes targeting non-executable linesHow to test the change?
Unit tests added for C extension functions (
all_iseqs,exception_message). Unit tests forserialize_throwablecover: exception present/absent, overridden message, non-string arg, nil arg. Integration test verifies full path from method probe exception capture through to snapshot with populated throwable. Manual verification screenshot included below.Screenshot of a manual test with a non-string Exception argument: