Skip to content

DI: add a C extension#5111

Merged
p-datadog merged 37 commits intomasterfrom
di-c-ext
Mar 27, 2026
Merged

DI: add a C extension#5111
p-datadog merged 37 commits intomasterfrom
di-c-ext

Conversation

@p-datadog
Copy link
Copy Markdown
Member

@p-datadog p-datadog commented Dec 5, 2025

What does this PR do?
Adds two C extension functions to libdatadog_api for Dynamic Instrumentation:

  1. all_iseqs: returns RubyVM::InstructionSequence objects 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.
  2. exception_message: returns the raw argument passed to the Exception constructor, bypassing any Ruby-level message method overrides.

Integrates exception_message into ProbeNotificationBuilder so method probe snapshots now populate the throwable field (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 from spec:main to the new spec:di:di_with_ext task, which compiles the extension before running tests. The task is added to the Matrixfile so CI runs them.

Motivation:

  • Line probes require iseq objects that the VM does not expose to Ruby code; the C extension provides access.
  • DI must never invoke customer code; exception_message accesses the raw C-level message, bypassing overridden message methods.
  • The C extension always compiles on MRI (the only platform DI supports), so there is no need for fallback code paths. Making it a hard requirement simplifies the codebase and makes failures explicit.

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_callback in 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:

How to test the change?
Unit tests added for C extension functions (all_iseqs, exception_message). Unit tests for serialize_throwable cover: 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:

2026-03-24_18-31-45

@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 5, 2025

Thank you for updating Change log entry section 👏

Visited at: 2025-12-05 17:39:23 UTC

@p-datadog p-datadog mentioned this pull request Dec 5, 2025
@pr-commenter
Copy link
Copy Markdown

pr-commenter bot commented Dec 5, 2025

Benchmarks

Benchmark execution time: 2026-03-27 16:34:19

Comparing candidate commit 6d63f39 in PR branch di-c-ext with baseline commit ec56ebe in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 0 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 force-pushed the di-c-ext branch 2 times, most recently from feef350 to 8be2f98 Compare December 8, 2025 21:25
@p-datadog p-datadog marked this pull request as ready for review December 8, 2025 21:26
@p-datadog p-datadog requested review from a team as code owners December 8, 2025 21:26
@datadog-datadog-prod-us1
Copy link
Copy Markdown
Contributor

datadog-datadog-prod-us1 bot commented Dec 8, 2025

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 99.53%
Overall Coverage: 96.33% (+1.16%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 3539fa7 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback!

@ivoanjo
Copy link
Copy Markdown
Member

ivoanjo commented Dec 9, 2025

The C code is in libdatadog_api extension, however it does not use libdatadog. This was done to not create another extension.

I think this is the right decision -- managing multiple extensions adds complexity AND makes installation slower.

Copy link
Copy Markdown
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

This is cool! Left a few notes :)

Comment thread ext/libdatadog_api/ddsketch.h Outdated
Comment thread ext/libdatadog_api/ruby_helpers.c Outdated
Comment thread ext/libdatadog_api/ruby_internal.h Outdated
Comment thread ext/libdatadog_api/ruby_helpers.c Outdated
Comment thread ext/libdatadog_api/ruby_helpers.c Outdated
Comment thread ext/libdatadog_api/di.c Outdated
Comment thread ext/libdatadog_api/di.c Outdated
Comment thread ext/libdatadog_api/di.c Outdated
Comment thread ext/libdatadog_api/di.c Outdated
@p-datadog p-datadog marked this pull request as draft March 9, 2026 17:46
@p-datadog p-datadog changed the title DI: add a C extension DI: add a C extension [no-restart] Mar 11, 2026
@p-datadog p-datadog changed the title DI: add a C extension [no-restart] DI: add a C extension Mar 11, 2026
@p-datadog p-datadog closed this Mar 11, 2026
@p-datadog p-datadog reopened this Mar 23, 2026
p-ddsign and others added 3 commits March 23, 2026 18:45
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>
@p-datadog
Copy link
Copy Markdown
Member Author

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>
p-ddsign and others added 3 commits March 24, 2026 16:22
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>
p-ddsign and others added 2 commits March 24, 2026 18:08
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>
p-ddsign and others added 6 commits March 24, 2026 20:37
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>
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>
p-ddsign and others added 2 commits March 25, 2026 00:43
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>
@Strech
Copy link
Copy Markdown
Member

Strech commented Mar 27, 2026

@codex Review for bugs and edge-cases

Copy link
Copy Markdown

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

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

Comment thread lib/datadog/di/probe_notification_builder.rb
Comment thread lib/datadog/di/probe_notification_builder.rb Outdated
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>
@p-datadog p-datadog merged commit 609f934 into master Mar 27, 2026
464 checks passed
@p-datadog p-datadog deleted the di-c-ext branch March 27, 2026 18:02
@github-actions github-actions bot added this to the 2.31.0 milestone Mar 27, 2026
@y9v y9v mentioned this pull request Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

profiling Involves Datadog profiling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants