Skip to content

Commit 3fef0d3

Browse files
committed
[PROF-3425] Bootstrap profiling native extension
My current plan is to use the profiling native extension to: * Enable use of libddprof, a (native) shared library which allows datadog profilers to share common code. One of the main advantages of this library is that it will include its own profile encoding implementation, which will enable us to drop profiler's dependency on the `google-protobuf` gem. Right now, we need to tell customers to manually it when onboarding, see <https://docs.datadoghq.com/tracing/profiler/enabling/?code-lang=ruby>, which is annoying. * Call Ruby VM APIs that are otherwise unavailable or costly when called from Ruby code. But in this commit, I decided to scale it way, way, way back to just the basics: add an empty native extension, and the scaffolding to load and test it. Thus, I hope that by releasing the next version of dd-trace-rb with the empty native extension we can to validate the approach, or otherwise root out corner cases that we may have missed. Furthermore, I'll point out that if our plans of a "big gem" go forward, having this kind of non-trivial addition on the gem is supposed to be the norm, not the exception ;) --- EVEN SO, because this is a non-trivial change, here's my notes on possible concerns, in Q&A form: **Q1**: Will requiring customers to compile a native extension during `gem install ddtrace` cause issues? **A1**: No, because of our dependencies. dd-trace-rb currently has two dependencies: `ffi` and `msgpack`. Both of those gems rely on native components, and neither of them (as of this writing) ships pre-compiled extensions (*except on Windows), as can be seen on rubygems.org: * <https://rubygems.org/gems/ffi/versions> * <https://rubygems.org/gems/msgpack/versions> This fortunate state of affairs means that customers already need to have a working setup for building native extensions, and so our addition of a native extension does not make it any harder for them to onboard. **Q2**: Will this cause problems for Windows users? **A2**: The `ffi` and `msgpack` gem ship precompiled binaries for Windows, so the reasoning from A1 doesn't apply to these users. For Windows, it's possible that customers that previously were getting by without needing an environment to build Ruby native extensions will no longer be able to install dd-trace-rb. But: * `gem install rails` on Windows pulls at least one native extension that needs to be compiled, so if you can't build dd-trace-rb, you can't install `rails` either * Recent versions of `msgpack` (since 1.4.2, from 2021-02-01) don't provide binaries either. This means that, out of the box, even before this change, `gem install ddtrace` fails on Windows if you don't have a build environment, because rubygems tries to download the latest version of `msgpack`. (Rather than picking an older version that ships a precompiled build.) So my assertion is, I don't believe we'll have any customers that A) run on Windows and B) don't have a setup for building native extensions. BUT, if this assertion turns out to be wrong, we have the option of doing the same thing that `ffi` and `msgpack` do: ship prebuilt versions of `ddtrace` for Windows users. **Q3**: Should we provide precompiled versions of the gem right now instead? **A3**: Precompiled versions of the gem introduce complexity into our release process (now we have several artifacts, that may need to be generated on multiple machines); it also may pose compatibility issues, given our Ruby 2.1 to 3.0 support Matrix. So, given the fortunate state we're in (see A1), I think we should avoid them as much as possible. **Q4**: Why write the extension in C and not Rust? **A4**: The Ruby VM APIs and headers are written in C. They cannot be directly used from Rust (e.g. a few things are actually implemented as preprocessor macros), and thus, to write an extension using Rust, we'd need to rely on community-built bindings that translate the Ruby VM headers into Rust. I've investigated the state of these bindings, and the only two that are still maintained are: * https://crates.io/crates/rosy * https://crates.io/crates/rutie Unfortunately, there don't seem to be a lot of gems using them and support for older Rubies, 32-bit OSs and Windows seems spotty. So... not in a great state at the moment for our usage. The second issue is that using Rust pushes us into needing to provide binary builds through rubygems.org -- we definitely can't assume that customers will have a working Rust compiler around. We plan on implementing libddprof (the profiling shared library) using Rust, but because it doesn't need to talk to Ruby VM APIs (we'll wrap them with some C code in this profiling native extension), we don't need to worry about bindings, and also we get a bit more flexibility on binary builds, since we don't need to link to the Ruby VM from Rust code. **Q5**: Can you use dd-trace-rb without the native extension? **A5**: Yes...ish. The extension must get built during `gem install`, but we handle any issues that may happen while loading it. So, if you're working on the gem, or the extension gets built but doesn't load properly, there should be no impact to the rest of the library; only the profiler will refuse to work. **Q6**: Will this impact JRuby users? **A6**: No. We'll skip trying to compile and load the native extension on JRuby. (Profiling is anyway not supported on JRuby).
1 parent 55ffdfe commit 3fef0d3

File tree

13 files changed

+215
-5
lines changed

13 files changed

+215
-5
lines changed

.editorconfig

+22
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
# EditorConfig is awesome: https://EditorConfig.org
2+
3+
# top-most EditorConfig file
4+
root = true
5+
6+
# Unix-style newlines with a newline ending every file
7+
[*]
8+
end_of_line = lf
9+
insert_final_newline = true
10+
trim_trailing_whitespace = true
11+
12+
[*.h]
13+
indent_style = space
14+
indent_size = 2
15+
16+
[*.c]
17+
indent_style = space
18+
indent_size = 2
19+
20+
[*.yml]
21+
indent_style = space
22+
indent_size = 2

.gitignore

+4
Original file line numberDiff line numberDiff line change
@@ -58,3 +58,7 @@ Gemfile.lock
5858

5959
# bundle config
6060
gemfiles/.bundle
61+
62+
# Native extension binaries
63+
lib/*.bundle
64+
lib/*.so

.rubocop.yml

+1
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ AllCops:
88
TargetRubyVersion: 2.5 # Lowest version supported as of Rubocop 0.13.0
99
Include:
1010
- 'lib/**/*.rb'
11+
- 'ext/**/*.rb'
1112
- 'test/**/*.rb'
1213
- 'spec/**/*.rb'
1314
- 'Gemfile'

Gemfile

+1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ else
2626
gem 'pry-debugger-jruby'
2727
end
2828
gem 'rake', '>= 10.5', '< 13.0.4' # Locking rake until https://github.com/thoughtbot/appraisal/pull/184 is released
29+
gem 'rake-compiler', '~> 1.1', '>= 1.1.1' # To compile native extensions
2930
gem 'redcarpet', '~> 3.4' if RUBY_PLATFORM != 'java'
3031
gem 'rspec', '~> 3.10'
3132
gem 'rspec-collection_matchers', '~> 1.1'

Rakefile

+6
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ require 'bundler/gem_tasks'
22
require 'ddtrace/version'
33
require 'rubocop/rake_task' if Gem.loaded_specs.key? 'rubocop'
44
require 'rspec/core/rake_task'
5+
require 'rake/extensiontask'
56
require 'rake/testtask'
67
require 'appraisal'
78
require 'yard'
@@ -21,6 +22,7 @@ namespace :spec do
2122
' spec/**/auto_instrument_spec.rb'
2223
t.rspec_opts = args.to_a.join(' ')
2324
end
25+
Rake::Task[:main].enhance([:compile]) if RUBY_PLATFORM != 'java' # Compile native extensions before running the main task
2426

2527
RSpec::Core::RakeTask.new(:benchmark) do |t, args|
2628
t.pattern = 'spec/ddtrace/benchmark/**/*_spec.rb'
@@ -979,4 +981,8 @@ namespace :changelog do
979981
end
980982
end
981983

984+
Rake::ExtensionTask.new("ddtrace_profiling_native_extension.#{RUBY_VERSION}_#{RUBY_PLATFORM}") do |ext|
985+
ext.ext_dir = 'ext/ddtrace_profiling_native_extension'
986+
end
987+
982988
task default: 'spec:main'

ddtrace.gemspec

+2
Original file line numberDiff line numberDiff line change
@@ -41,4 +41,6 @@ Gem::Specification.new do |spec|
4141

4242
# Used by the profiler
4343
spec.add_dependency 'ffi', '~> 1.0'
44+
45+
spec.extensions = ['ext/ddtrace_profiling_native_extension/extconf.rb']
4446
end
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
require 'mkmf'
2+
3+
# We don't support JRuby for profiling, and JRuby doesn't support native extensions, so let's just skip this entire
4+
# thing so that JRuby users of dd-trace-rb aren't impacted.
5+
if RUBY_ENGINE == 'jruby'
6+
File.write('Makefile', dummy_makefile($srcdir).join) # rubocop:disable Style/GlobalVars
7+
return
8+
end
9+
10+
# Tag the native extension library with the Ruby version and Ruby platform.
11+
# This makes it easier for development (avoids "oops I forgot to rebuild when I switched my Ruby") and ensures that
12+
# the wrong library is never loaded.
13+
# When requiring, we need to use the exact same string, including the version and the platform.
14+
create_makefile "ddtrace_profiling_native_extension.#{RUBY_VERSION}_#{RUBY_PLATFORM}"
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
#include <ruby.h>
2+
#include <stdio.h>
3+
4+
static VALUE native_working_p(VALUE self);
5+
6+
void Init_ddtrace_profiling_native_extension(void) {
7+
VALUE datadog_module = rb_define_module("Datadog");
8+
VALUE profiling_module = rb_define_module_under(datadog_module, "Profiling");
9+
VALUE native_extension_module = rb_define_module_under(profiling_module, "NativeExtension");
10+
11+
rb_define_singleton_method(native_extension_module, "native_working?", native_working_p, 0);
12+
rb_funcall(native_extension_module, rb_intern("private_class_method"), 1, ID2SYM(rb_intern("native_working?")));
13+
}
14+
15+
static VALUE native_working_p(VALUE self) {
16+
return Qtrue;
17+
}

lib/ddtrace/profiling.rb

+32-2
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@ def unsupported_reason
1414
# NOTE: Only the first matching reason is returned, so try to keep a nice order on reasons -- e.g. tell users
1515
# first that they can't use this on JRuby before telling them that they are missing protobuf
1616

17-
ruby_engine_unsupported? || protobuf_gem_unavailable? || protobuf_version_unsupported? || protobuf_failed_to_load?
17+
ruby_engine_unsupported? ||
18+
native_library_failed_to_load? ||
19+
protobuf_gem_unavailable? ||
20+
protobuf_version_unsupported? ||
21+
protobuf_failed_to_load?
1822
end
1923

2024
def self.ruby_engine_unsupported?
@@ -81,6 +85,32 @@ def self.protobuf_loaded_successfully?
8185
end
8286
private_class_method :protobuf_loaded_successfully?
8387

88+
private_class_method def self.native_library_failed_to_load?
89+
success, exception = try_loading_native_library
90+
91+
unless success
92+
if exception
93+
'There was an error loading the profiling native extension due to ' \
94+
"'#{exception.message}' at '#{exception.backtrace.first}'"
95+
else
96+
'The profiling native extension did not load correctly. ' \
97+
'If the error persists, please contact support via <https://docs.datadoghq.com/help/> or ' \
98+
'file a bug at <https://github.com/DataDog/dd-trace-rb/blob/master/CONTRIBUTING.md#found-a-bug>.'
99+
end
100+
end
101+
end
102+
103+
private_class_method def self.try_loading_native_library
104+
begin
105+
require "ddtrace_profiling_native_extension.#{RUBY_VERSION}_#{RUBY_PLATFORM}"
106+
success =
107+
defined?(Datadog::Profiling::NativeExtension) && Datadog::Profiling::NativeExtension.send(:native_working?)
108+
[success, nil]
109+
rescue StandardError, LoadError => e
110+
[false, e]
111+
end
112+
end
113+
84114
def self.load_profiling
85115
return false unless supported?
86116

@@ -95,7 +125,7 @@ def self.load_profiling
95125
require 'ddtrace/profiling/transport/io'
96126
require 'ddtrace/profiling/transport/http'
97127
require 'ddtrace/profiling/profiler'
98-
128+
require 'ddtrace/profiling/native_extension'
99129
require 'ddtrace/profiling/pprof/pprof_pb'
100130

101131
true
+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
module Datadog
2+
module Profiling
3+
# This module contains classes and methods which are implemented using native code in the
4+
# ext/ddtrace_profiling_native_extension folder
5+
module NativeExtension
6+
private_class_method def self.working?
7+
native_working?
8+
end
9+
10+
unless singleton_class.private_method_defined?(:native_working?)
11+
private_class_method def self.native_working?
12+
false
13+
end
14+
end
15+
end
16+
end
17+
end

spec/ddtrace/auto_instrument_spec.rb

+1-1
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ def migrate_db
113113
it 'starts the profiler' do
114114
skip 'Profiler not supported on JRuby' if PlatformHelpers.jruby?
115115

116-
profiler = instance_double(Datadog::Profiler)
116+
profiler = instance_double('Datadog::Profiler')
117117

118118
expect(Datadog).to receive(:profiler).and_return(profiler).at_least(:once)
119119
expect(profiler).to receive(:start)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
require 'ddtrace/profiling/native_extension'
2+
3+
RSpec.describe Datadog::Profiling::NativeExtension do
4+
before do
5+
skip('Profiling not supported on JRuby') if PlatformHelpers.jruby?
6+
7+
begin
8+
require "ddtrace_profiling_native_extension.#{RUBY_VERSION}_#{RUBY_PLATFORM}"
9+
rescue LoadError
10+
raise 'Profiling native extension does not seem to be compiled. ' \
11+
'Try running `bundle exec rake compile` before running this test.'
12+
end
13+
end
14+
15+
describe '.working?' do
16+
subject(:working?) { described_class.send(:working?) }
17+
18+
it { is_expected.to be true }
19+
end
20+
end

spec/ddtrace/profiling_spec.rb

+78-2
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,35 @@
3232
context 'when not using JRuby' do
3333
before { stub_const('RUBY_ENGINE', 'ruby') }
3434

35-
context 'and \'google-protobuf\'' do
35+
context 'when the profiling native library fails to be loaded with a exception' do
36+
let(:loaderror) do
37+
begin
38+
raise LoadError, 'Simulated require failure'
39+
rescue LoadError => e
40+
e
41+
end
42+
end
43+
44+
before do
45+
expect(described_class).to receive(:try_loading_native_library).and_return([false, loaderror])
46+
end
47+
48+
it { is_expected.to include 'error loading the profiling native extension' }
49+
end
50+
51+
context "when the profiling native library fails to be loaded but there's no exception" do
52+
before do
53+
expect(described_class).to receive(:try_loading_native_library).and_return [false, nil]
54+
end
55+
56+
it { is_expected.to include 'profiling native extension did not load correctly' }
57+
end
58+
59+
context "when the profiling native library is available and 'google-protobuf'" do
60+
before do
61+
expect(described_class).to receive(:try_loading_native_library).and_return [true, nil]
62+
end
63+
3664
context 'is not available' do
3765
include_context 'loaded gems', :'google-protobuf' => nil
3866

@@ -101,7 +129,7 @@
101129
it { is_expected.to be false }
102130

103131
it 'logs a warning' do
104-
expect(Kernel).to receive(:warn).with(/Error while loading/)
132+
expect(Kernel).to receive(:warn).with(/Error while loading google-protobuf/)
105133

106134
protobuf_loaded_successfully?
107135
end
@@ -113,4 +141,52 @@
113141
it { is_expected.to be true }
114142
end
115143
end
144+
145+
describe '::try_loading_native_library' do
146+
subject(:try_loading_native_library) { described_class.send(:try_loading_native_library) }
147+
148+
let(:native_extension_require) { "ddtrace_profiling_native_extension.#{RUBY_VERSION}_#{RUBY_PLATFORM}" }
149+
150+
context 'when the profiling native library loads successfully' do
151+
before do
152+
expect(described_class)
153+
.to receive(:require)
154+
.with(native_extension_require)
155+
stub_const('Datadog::Profiling::NativeExtension', double(native_working?: true))
156+
end
157+
158+
it { is_expected.to eq [true, nil] }
159+
end
160+
161+
context 'when the profiling native library fails to load with a LoadError' do
162+
before do
163+
expect(described_class).to receive(:require).with(native_extension_require).and_raise(loaderror)
164+
end
165+
166+
let(:loaderror) { LoadError.new('Simulated require failure') }
167+
168+
it { is_expected.to eq [false, loaderror] }
169+
end
170+
171+
context 'when the profiling native library fails to load with a different error' do
172+
before do
173+
expect(described_class).to receive(:require).with(native_extension_require).and_raise(error)
174+
end
175+
176+
let(:error) { StandardError.new('Simulated require failure') }
177+
178+
it { is_expected.to eq [false, error] }
179+
end
180+
181+
context 'when the profiling native library loads but does not install code correctly' do
182+
before do
183+
expect(described_class)
184+
.to receive(:require)
185+
.with(native_extension_require)
186+
stub_const('Datadog::Profiling::NativeExtension', double(native_working?: false))
187+
end
188+
189+
it { is_expected.to eq [false, nil] }
190+
end
191+
end
116192
end

0 commit comments

Comments
 (0)