Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PROF-8917] Add support for the libdatadog crash tracker #3384

Merged
merged 48 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
e1340de
Introduce setting to control crash tracker option
ivoanjo Jan 16, 2024
789db6b
Extract `convert_tags` and `endpoint_from` from `HttpTransport` to li…
ivoanjo Jan 16, 2024
8afc9f2
Extract `ddtrace_version` from `HttpTransport` to `ruby_helpers.h`
ivoanjo Jan 16, 2024
a9c51b0
[PROF-8917] Add support for the libdatadog crash tracker
ivoanjo Jan 16, 2024
d89e876
Add TODO about integration spec
ivoanjo Jan 16, 2024
d3bc290
Update `crash_tracker.c` with latest libdatadog API
ivoanjo Mar 11, 2024
4889d05
Add experimental spec
ivoanjo Mar 11, 2024
37f8082
Minor: Remove unused/outdated type declaration
ivoanjo Mar 11, 2024
18758de
Redesign crash tracker to behave as regular object
ivoanjo Mar 11, 2024
02d8691
Merge branch 'master' into ivoanjo/prof-8917-crash-tracker-ruby
ivoanjo Mar 12, 2024
bc7d72c
Wire new crash tracker design into profiler
ivoanjo Mar 12, 2024
5312591
Minor: Remove redundant log message
ivoanjo Mar 12, 2024
45f5daa
Avoid leaking threads and outputting errors during spec
ivoanjo Mar 12, 2024
a5c0ba2
Rename CrashTracker to Crashtracker to match libdatadog naming
ivoanjo Mar 12, 2024
fb45524
Match fixed case for crashtracker APIs
ivoanjo Mar 12, 2024
6f83a4a
Merge branch 'master' into ivoanjo/prof-8917-crash-tracker-ruby
ivoanjo Mar 12, 2024
6af57da
Update to libdatadog 7 APIs
ivoanjo Mar 22, 2024
d19ecde
Merge branch 'master' into ivoanjo/prof-8917-crash-tracker-ruby
ivoanjo Mar 22, 2024
e670699
Enable frame resolution
ivoanjo Mar 22, 2024
d37cbb8
Use in-receiver resolve frames
ivoanjo Mar 28, 2024
d8b4122
Merge branch 'master' into ivoanjo/prof-8917-crash-tracker-ruby
ivoanjo Apr 3, 2024
6f3f8be
Adjust to latest libdatadog crash tracker changes
ivoanjo Apr 3, 2024
66e8d95
Remove TODO, it's fixed now
ivoanjo Apr 3, 2024
ce15ade
Add detail to explaining why alt stack can't be used
ivoanjo Apr 4, 2024
1b501cc
Clarify section of method where exceptions mustn't be raised
ivoanjo Apr 4, 2024
bd0537e
Document that crashtracker state is a singleton
ivoanjo Apr 4, 2024
0b40fbf
Avoid hardcoding ports when testing with built-in webrick
ivoanjo Apr 4, 2024
a3081b4
Add test coverage for crashtracker surviving in `#component_failed`
ivoanjo Apr 4, 2024
680c4a3
[NO-TICKET] Upgrade to libdatadog 8
ivoanjo Apr 9, 2024
32f02b4
Update gemfiles with libdatadog 7 -> 8 upgrade
ivoanjo Apr 9, 2024
356002e
Merge branch 'ivoanjo/libdatadog8-upgrade' into ivoanjo/prof-8917-cra…
ivoanjo Apr 9, 2024
7776107
[NO-TICKET] Upgrade to libdatadog 9
AlexJF May 6, 2024
520c8ab
Restore `required_ruby_version` to be in single line, but make Ruboco…
ivoanjo May 9, 2024
a6ddf72
Minor cleanups to comments
ivoanjo May 9, 2024
3c27b38
Minor: Fix length of guide comment for breaking lines in message
ivoanjo May 9, 2024
ed971a5
Revert "[NO-TICKET] Upgrade to libdatadog 8"
ivoanjo May 9, 2024
f029683
Revert "Update gemfiles with libdatadog 7 -> 8 upgrade"
ivoanjo May 9, 2024
7083455
Merge branch 'alexjf/libdatadog9' into ivoanjo/prof-8917-crash-tracke…
ivoanjo May 9, 2024
49e9f31
Update Ruby crashtracker to libdatadog v9 API
ivoanjo May 9, 2024
5197792
Setup `ld_library_path` argument for crashtracker
ivoanjo May 9, 2024
7cda332
Use `profiling.upload.timeout_seconds` for crashtracker timeout
ivoanjo May 9, 2024
b87f171
Remove temporary libdatadog monkey patch
ivoanjo May 9, 2024
3acf413
Fix `upload_timeout_seconds` being a float by default
ivoanjo May 9, 2024
bd7de08
Make rubocop happy
ivoanjo May 9, 2024
09976cc
Minor: Add explanation for why we're skipping crash tracker with cust…
ivoanjo May 10, 2024
5431d2b
Assert that no crashtracker is running before each test
ivoanjo May 10, 2024
a2ae730
Assert that correct signal name is reported
ivoanjo May 10, 2024
42f6ca5
Minor: Empty commit to re-trigger CI
ivoanjo May 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Setup ld_library_path argument for crashtracker
  • Loading branch information
ivoanjo committed May 9, 2024
commit 5197792fd98af8c34ef5549e2f9d776aa6ddae75
8 changes: 5 additions & 3 deletions ext/datadog_profiling_native_extension/crashtracker.c
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ static VALUE _native_start_or_update_on_fork(int argc, VALUE *argv, DDTRACE_UNUS

VALUE exporter_configuration = rb_hash_fetch(options, ID2SYM(rb_intern("exporter_configuration")));
VALUE path_to_crashtracking_receiver_binary = rb_hash_fetch(options, ID2SYM(rb_intern("path_to_crashtracking_receiver_binary")));
VALUE ld_library_path = rb_hash_fetch(options, ID2SYM(rb_intern("ld_library_path")));
VALUE tags_as_array = rb_hash_fetch(options, ID2SYM(rb_intern("tags_as_array")));
VALUE action = rb_hash_fetch(options, ID2SYM(rb_intern("action")));
VALUE start_action = ID2SYM(rb_intern("start"));
Expand All @@ -29,6 +30,7 @@ static VALUE _native_start_or_update_on_fork(int argc, VALUE *argv, DDTRACE_UNUS
ENFORCE_TYPE(exporter_configuration, T_ARRAY);
ENFORCE_TYPE(tags_as_array, T_ARRAY);
ENFORCE_TYPE(path_to_crashtracking_receiver_binary, T_STRING);
ENFORCE_TYPE(ld_library_path, T_STRING);
ENFORCE_TYPE(action, T_SYMBOL);

if (action != start_action && action != update_on_fork_action) rb_raise(rb_eArgError, "Unexpected action: %+"PRIsVALUE, action);
Expand Down Expand Up @@ -63,14 +65,14 @@ static VALUE _native_start_or_update_on_fork(int argc, VALUE *argv, DDTRACE_UNUS
.tags = &tags,
};

ddog_prof_EnvVar ld_library_path = {
ddog_prof_EnvVar ld_library_path_env = {
.key = DDOG_CHARSLICE_C("LD_LIBRARY_PATH"),
.val = DDOG_CHARSLICE_C("FIXME"), // FIXME
.val = char_slice_from_ruby_string(ld_library_path),
};

ddog_prof_CrashtrackerReceiverConfig receiver_config = {
.args = {},
.env = {.ptr = &ld_library_path, .len = 1},
.env = {.ptr = &ld_library_path_env, .len = 1},
Copy link

Choose a reason for hiding this comment

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

I think this might override the env rather than appending. Which would you expect to happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question -- I don't particularly have a preference.

On one side, inheriting the full env would be useful if the crash tracker wanted to log some extra info from there. On the other hand, we can always add anything else we want extra later.

Dealer's choice? Do you think it'd be useful for me to pass along the env that Ruby has + with the addition of the ld_library_path?

.path_to_receiver_binary = char_slice_from_ruby_string(path_to_crashtracking_receiver_binary),
.optional_stderr_filename = {},
.optional_stdout_filename = {},
Expand Down
25 changes: 23 additions & 2 deletions lib/datadog/profiling/crashtracker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,17 @@

require 'libdatadog'

# Temporary, this should be moved to libdatadog
module Libdatadog
def self.ld_library_path
pkgconfig_folder = self.pkgconfig_folder

return unless pkgconfig_folder

File.absolute_path("#{pkgconfig_folder}/../")
end
end

module Datadog
module Profiling
# Used to report Ruby VM crashes.
Expand All @@ -15,18 +26,20 @@ module Profiling
class Crashtracker
private

attr_reader :exporter_configuration, :tags_as_array, :path_to_crashtracking_receiver_binary
attr_reader :exporter_configuration, :tags_as_array, :path_to_crashtracking_receiver_binary, :ld_library_path

public

def initialize(
exporter_configuration:,
tags:,
path_to_crashtracking_receiver_binary: Libdatadog.path_to_crashtracking_receiver_binary
path_to_crashtracking_receiver_binary: Libdatadog.path_to_crashtracking_receiver_binary,
ld_library_path: Libdatadog.ld_library_path
)
@exporter_configuration = exporter_configuration
@tags_as_array = tags.to_a
@path_to_crashtracking_receiver_binary = path_to_crashtracking_receiver_binary
@ld_library_path = ld_library_path
end

def start
Expand Down Expand Up @@ -56,11 +69,19 @@ def start_or_update_on_fork(action:)
return
end

unless ld_library_path
Datadog.logger.warn(
"Cannot #{action} profiling crash tracking as no ld_library_path was found"
)
return
end

begin
self.class._native_start_or_update_on_fork(
action: action,
exporter_configuration: exporter_configuration,
path_to_crashtracking_receiver_binary: path_to_crashtracking_receiver_binary,
ld_library_path: ld_library_path,
tags_as_array: tags_as_array,
)
Datadog.logger.debug("Crash tracking #{action} successful")
Expand Down
5 changes: 4 additions & 1 deletion sig/datadog/profiling/crashtracker.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ module Datadog
attr_reader exporter_configuration: exporter_configuration_array
attr_reader tags_as_array: ::Array[[::String, ::String]]
attr_reader path_to_crashtracking_receiver_binary: ::String
attr_reader ld_library_path: ::String

public

def initialize: (
exporter_configuration: exporter_configuration_array,
tags: ::Hash[::String, ::String],
?path_to_crashtracking_receiver_binary: ::String,
?ld_library_path: ::String,
) -> void

def start: -> void
Expand All @@ -28,8 +30,9 @@ module Datadog
def self._native_start_or_update_on_fork: (
action: :start | :update_on_fork,
exporter_configuration: exporter_configuration_array,
tags_as_array: ::Array[[::String, ::String]],
path_to_crashtracking_receiver_binary: ::String,
ld_library_path: ::String,
tags_as_array: ::Array[[::String, ::String]],
) -> void

def self._native_stop: -> void
Expand Down
16 changes: 16 additions & 0 deletions spec/datadog/profiling/crashtracker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,22 @@
end
end

context 'when ld_library_path is nil' do
subject(:crashtracker) do
described_class.new(
exporter_configuration: exporter_configuration,
tags: { 'tag1' => 'value1', 'tag2' => 'value2' },
ld_library_path: nil
)
end

it 'logs a warning' do
expect(Datadog.logger).to receive(:warn).with(/no ld_library_path was found/)

start
end
end

it 'starts the crash tracker' do
start

Expand Down
1 change: 1 addition & 0 deletions vendor/rbs/libdatadog/0/libdatadog.rbs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
module Libdatadog
def self.path_to_crashtracking_receiver_binary: () -> ::String?
def self.ld_library_path: () -> ::String?
def self.pkgconfig_folder: () -> ::String?
end