Create shared sources directory for `ext/` #5088

marcotc · 2025-11-25T00:45:11Z

What does this PR do?

Motivation:

Change log entry

Additional Notes:

How to test the change?

github-actions · 2025-11-25T00:45:23Z

👋 Hey @marcotc, 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: 2025-11-25 00:45:22 UTC}

datadog-datadog-prod-us1 · 2025-11-25T01:03:01Z

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage
• Patch Coverage: 9.09%
• Total Coverage: 95.16% (-0.01%)

View detailed report

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 6b3bf9b | Docs | Datadog PR Page | Was this helpful? Give us feedback!}

pr-commenter · 2025-11-25T01:15:39Z

Benchmarks

Benchmark execution time: 2025-11-25 01:15:11

Comparing candidate commit 6b3bf9b in PR branch marcotc/dedup-extshare with baseline commit ae94edc in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 44 metrics, 2 unstable metrics.

ivoanjo

This is great! Let's ship it! Thanks for fixing up my past laziness :)

ivoanjo · 2025-11-25T09:41:47Z

ext/libdatadog_extconf_helpers.rb

+      shared_sources = Dir[File.join(shared_absolute_dir, '*.c')].map { |path| File.basename(path) }
+      extension_sources = Dir[File.join(extension_dir, '*.c')].map { |path| File.basename(path) }
+
+      $srcs = extension_sources + shared_sources


So I added this to the bottom of the extconf.rb so I could compare between master and this branch:

$stderr.puts("$srcs: #{$srcs.inspect}") $stderr.puts("$VPATH: #{$objs.inspect}") $stderr.puts("$INCFLAGS: #{$INCFLAGS.inspect}")

and I got this on master:

$srcs: ["../../../../ext/datadog_profiling_native_extension/clock_id_from_pthread.c", "../../../../ext/datadog_profiling_native_extension/clock_id_noop.c", "../../../../ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c", "../../../../ext/datadog_profiling_native_extension/collectors_discrete_dynamic_sampler.c", "../../../../ext/datadog_profiling_native_extension/collectors_dynamic_sampling_rate.c", "../../../../ext/datadog_profiling_native_extension/collectors_gc_profiling_helper.c", "../../../../ext/datadog_profiling_native_extension/collectors_idle_sampling_helper.c", "../../../../ext/datadog_profiling_native_extension/collectors_stack.c", "../../../../ext/datadog_profiling_native_extension/collectors_thread_context.c", "../../../../ext/datadog_profiling_native_extension/datadog_ruby_common.c", "../../../../ext/datadog_profiling_native_extension/encoded_profile.c", "../../../../ext/datadog_profiling_native_extension/gvl_profiling_helper.c", "../../../../ext/datadog_profiling_native_extension/heap_recorder.c", "../../../../ext/datadog_profiling_native_extension/http_transport.c", "../../../../ext/datadog_profiling_native_extension/libdatadog_helpers.c", "../../../../ext/datadog_profiling_native_extension/private_vm_api_access.c", "../../../../ext/datadog_profiling_native_extension/profiling.c", "../../../../ext/datadog_profiling_native_extension/ruby_helpers.c", "../../../../ext/datadog_profiling_native_extension/setup_signal_handler.c", "../../../../ext/datadog_profiling_native_extension/stack_recorder.c", "../../../../ext/datadog_profiling_native_extension/time_helpers.c", "../../../../ext/datadog_profiling_native_extension/unsafe_api_calls_check.c"]
$VPATH: ["clock_id_from_pthread.o", "clock_id_noop.o", "collectors_cpu_and_wall_time_worker.o", "collectors_discrete_dynamic_sampler.o", "collectors_dynamic_sampling_rate.o", "collectors_gc_profiling_helper.o", "collectors_idle_sampling_helper.o", "collectors_stack.o", "collectors_thread_context.o", "datadog_ruby_common.o", "encoded_profile.o", "gvl_profiling_helper.o", "heap_recorder.o", "http_transport.o", "libdatadog_helpers.o", "private_vm_api_access.o", "profiling.o", "ruby_helpers.o", "setup_signal_handler.o", "stack_recorder.o", "time_helpers.o", "unsafe_api_calls_check.o"]
$INCFLAGS: "-I$(arch_hdrdir) -I$(hdrdir)/ruby/backward -I$(hdrdir) -I$(srcdir) -I/home/ivo.anjo/.rvm/gems/ruby-3.4.7/gems/libdatadog-24.0.1.1.0-x86_64-linux/vendor/libdatadog-24.0.1/x86_64-linux/libdatadog-x86_64-unknown-linux-gnu/lib/pkgconfig/../../include"

...and this on this branch:

$srcs: ["clock_id_from_pthread.c", "clock_id_noop.c", "collectors_cpu_and_wall_time_worker.c", "collectors_discrete_dynamic_sampler.c", "collectors_dynamic_sampling_rate.c", "collectors_gc_profiling_helper.c", "collectors_idle_sampling_helper.c", "collectors_stack.c", "collectors_thread_context.c", "encoded_profile.c", "gvl_profiling_helper.c", "heap_recorder.c", "http_transport.c", "libdatadog_helpers.c", "private_vm_api_access.c", "profiling.c", "ruby_helpers.c", "setup_signal_handler.c", "stack_recorder.c", "time_helpers.c", "unsafe_api_calls_check.c", "datadog_ruby_common.c"]
$VPATH: ["clock_id_from_pthread.o", "clock_id_noop.o", "collectors_cpu_and_wall_time_worker.o", "collectors_discrete_dynamic_sampler.o", "collectors_dynamic_sampling_rate.o", "collectors_gc_profiling_helper.o", "collectors_idle_sampling_helper.o", "collectors_stack.o", "collectors_thread_context.o", "encoded_profile.o", "gvl_profiling_helper.o", "heap_recorder.o", "http_transport.o", "libdatadog_helpers.o", "private_vm_api_access.o", "profiling.o", "ruby_helpers.o", "setup_signal_handler.o", "stack_recorder.o", "time_helpers.o", "unsafe_api_calls_check.o", "datadog_ruby_common.o"]
$INCFLAGS: "-I$(arch_hdrdir) -I$(hdrdir)/ruby/backward -I$(hdrdir) -I$(srcdir) -I$(srcdir)/../shared -I/home/ivo.anjo/.rvm/gems/ruby-3.4.7/gems/libdatadog-24.0.1.1.0-x86_64-linux/vendor/libdatadog-24.0.1/x86_64-linux/libdatadog-x86_64-unknown-linux-gnu/lib/pkgconfig/../../include"

I usually avoid the vague "let's do X 'just in case'" approach to things, but given:

We need to support many Ruby versions and there's many different ways of setting stuff up

It's easy to match exactly what Ruby does

My suggestion would be to setup $srcs exactly like Ruby does (e.g. with the relative paths from the RbConfig srcdir). I don't have a "oh this won't work like this reason", it's 100% just in case 😅

Sounds good!

I'll add some extra documentation for this section, regarding the pattern of following the upstream pattern.
I'll take a look too at a few other popular gems to see how they do it, and I'll document if I find anything interesting.

ivoanjo · 2025-11-25T09:52:33Z

ext/libdatadog_extconf_helpers.rb

+      $INCFLAGS ||= ''
+      include_token = " -I#{shared_search_dir}"
+      $INCFLAGS << include_token unless $INCFLAGS.include?(include_token)
+    end


I soft-suspected we may be missing something with the headers, since if we don't explicitly list all headers in the Makefile, make won't know to rebuild the code if the header changes (even though it can find the headers).

I diff'd the Makefile that gets generated before and after this change and noticed that indeed this was the case:

-HDRS = $(srcdir)/clock_id.h $(srcdir)/collectors_discrete_dynamic_sampler.h $(srcdir)/collectors_dynamic_sampling_rate.h $(srcdir)/collectors_gc_profiling_helper.h $(srcdir)/collectors_idle_sampling_helper.h $(srcdir)/collectors_stack.h $(srcdir)/collectors_thread_context.h $(srcdir)/datadog_ruby_common.h $(srcdir)/encoded_profile.h $(srcdir)/gvl_profiling_helper.h $(srcdir)/heap_recorder.h $(srcdir)/helpers.h $(srcdir)/libdatadog_helpers.h $(srcdir)/private_vm_api_access.h $(srcdir)/ruby_helpers.h $(srcdir)/setup_signal_handler.h $(srcdir)/stack_recorder.h $(srcdir)/time_helpers.h $(srcdir)/unsafe_api_calls_check.h +HDRS = $(srcdir)/clock_id.h $(srcdir)/collectors_discrete_dynamic_sampler.h $(srcdir)/collectors_dynamic_sampling_rate.h $(srcdir)/collectors_gc_profiling_helper.h $(srcdir)/collectors_idle_sampling_helper.h $(srcdir)/collectors_stack.h $(srcdir)/collectors_thread_context.h $(srcdir)/encoded_profile.h $(srcdir)/gvl_profiling_helper.h $(srcdir)/heap_recorder.h $(srcdir)/helpers.h $(srcdir)/libdatadog_helpers.h $(srcdir)/private_vm_api_access.h $(srcdir)/ruby_helpers.h $(srcdir)/setup_signal_handler.h $(srcdir)/stack_recorder.h $(srcdir)/time_helpers.h $(srcdir)/unsafe_api_calls_check.h

TL;DR I believe $headers is the correct thing to add to so the headers get added to the Makefile as well (I think they end up in a different definition, not HDRS, but it should be equivalent, and running bundle exec rake compile after changing only the header should trigger a rebuild)

ivoanjo · 2025-11-25T09:53:37Z

ext/libdatadog_extconf_helpers.rb

+    # * Keep every entry in $srcs to the basename so mkmf/VPATH can resolve them correctly.
+    # * Add the shared directory to $VPATH using $(srcdir) so the generated Makefile works no matter where it's run from.
+    # * Extend $INCFLAGS with the same path so headers resolve without absolute paths that would break relocation.
+    def self.add_shared_sources!(extension_dir:, shared_relative_dir:)


Minor: Maybe hardcode shared_relative_dir? I think for now it's easier to keep shared stuff together and we can always reintroduce this later.

ivoanjo · 2025-11-25T09:54:49Z

ext/libdatadog_extconf_helpers.rb

Btw if you want to add any specs (hint hint lol haha), there's already a libdatadog_extconf_helpers_spec.rb living inside profiling that we can use ;)

In the other comments, you brought up very good concerns that I didn't think of when writing this.
So I'll see if I can get as many of them as possible in tests, thank you so much for all the feedback!

Sounds good! TBH I think this is already in very good shape so feel free to split improvements into separate PRs vs waiting for a bigger PR with it all ;)

Create shared sources directory for ext/

6b3bf9b

github-actions bot added the profiling Involves Datadog profiling label Nov 25, 2025

ivoanjo reviewed Nov 25, 2025

View reviewed changes

lloeki marked this pull request as ready for review November 25, 2025 11:32

lloeki requested review from a team as code owners November 25, 2025 11:32

lloeki marked this pull request as draft November 25, 2025 11:33

marcotc mentioned this pull request Nov 28, 2025

Telemetry-safe error reporting for native extensions #5076

Draft

Create shared sources directory for ext/ #5088

Are you sure you want to change the base?

Create shared sources directory for ext/ #5088

Uh oh!

Conversation

marcotc commented Nov 25, 2025

Uh oh!

github-actions bot commented Nov 25, 2025

Uh oh!

datadog-datadog-prod-us1 bot commented Nov 25, 2025 • edited by datadog-official bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pr-commenter bot commented Nov 25, 2025

Benchmarks

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Create shared sources directory for `ext/` #5088

Create shared sources directory for `ext/` #5088

datadog-datadog-prod-us1 bot commented Nov 25, 2025 •

edited by datadog-official bot

Loading