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-10112] Simplify method names from actionview templates in profiler #3759

Merged
merged 1 commit into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 32 additions & 0 deletions ext/datadog_profiling_native_extension/collectors_stack.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ static VALUE _native_sample(
);
static void maybe_add_placeholder_frames_omitted(VALUE thread, sampling_buffer* buffer, char *frames_omitted_message, int frames_omitted_message_size);
static void record_placeholder_stack_in_native_code(sampling_buffer* buffer, VALUE recorder_instance, sample_values values, sample_labels labels);
static void maybe_trim_template_random_ids(ddog_CharSlice *name_slice, ddog_CharSlice *filename_slice);

void collectors_stack_init(VALUE profiling_module) {
VALUE collectors_module = rb_define_module_under(profiling_module, "Collectors");
Expand Down Expand Up @@ -199,6 +200,8 @@ void sample_thread(
ddog_CharSlice name_slice = char_slice_from_ruby_string(name);
ddog_CharSlice filename_slice = char_slice_from_ruby_string(filename);

maybe_trim_template_random_ids(&name_slice, &filename_slice);

bool top_of_the_stack = i == 0;

// When there's only wall-time in a sample, this means that the thread was not active in the sampled period.
Expand Down Expand Up @@ -268,6 +271,35 @@ void sample_thread(
);
}

// Rails's ActionView likes to dynamically generate method names with suffixed hashes/ids, resulting in methods with
// names such as "_app_views_layouts_explore_html_haml__2304485752546535910_211320".
// This makes these stacks not aggregate well, as well as being not-very-useful data.
// (Reference: https://github.com/rails/rails/blob/4fa56814f18fd3da49c83931fa773caa727d8096/actionview/lib/action_view/template.rb#L389 )
//
// This method trims these suffixes, so that we keep less data + the names correctly aggregate together.
static void maybe_trim_template_random_ids(ddog_CharSlice *name_slice, ddog_CharSlice *filename_slice) {
// Check filename doesn't end with ".rb"; templates are usually along the lines of .html.erb/.html.haml/...
if (filename_slice->len < 3 || memcmp(filename_slice->ptr + filename_slice->len - 3, ".rb", 3) == 0) return;

int pos = name_slice->len - 1;

// Let's match on something__number_number:
// Find start of id suffix from the end...
if (name_slice->ptr[pos] < '0' || name_slice->ptr[pos] > '9') return;

// ...now match a bunch of numbers and interspersed underscores
for (int underscores = 0; pos >= 0 && underscores < 2; pos--) {
if (name_slice->ptr[pos] == '_') underscores++;
else if (name_slice->ptr[pos] < '0' || name_slice->ptr[pos] > '9') return;
}

// Make sure there's something left before the underscores (hence the <= instead of <) + match the last underscore
if (pos <= 0 || name_slice->ptr[pos] != '_') return;

// If we got here, we matched on our pattern. Let's slice the length of the string to exclude it.
name_slice->len = pos;
}

static void maybe_add_placeholder_frames_omitted(VALUE thread, sampling_buffer* buffer, char *frames_omitted_message, int frames_omitted_message_size) {
ptrdiff_t frames_omitted = stack_depth_for(thread) - buffer->max_frames;

Expand Down
61 changes: 61 additions & 0 deletions spec/datadog/profiling/collectors/stack_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,67 @@ def call_sleep
end
end
end

context 'when sampling a stack with a dynamically-generated template method name' do
let(:method_name) { '_app_views_layouts_explore_html_haml__2304485752546535910_211320' }
let(:filename) { '/myapp/app/views/layouts/explore.html.haml' }
let(:dummy_template) { double('Dummy template object') }

let(:do_in_background_thread) do
# rubocop:disable Security/Eval
# rubocop:disable Style/EvalWithLocation
# rubocop:disable Style/DocumentDynamicEvalDefinition
eval(
%(
def dummy_template.#{method_name}(ready_queue)
ready_queue << true
sleep
end

proc { |ready_queue| dummy_template.#{method_name}(ready_queue) }
),
binding,
filename,
123456
)
# rubocop:enable Security/Eval
# rubocop:enable Style/EvalWithLocation
# rubocop:enable Style/DocumentDynamicEvalDefinition
end

it 'has a frame with a simplified method name' do
expect(gathered_stack).to include(
have_attributes(
path: '/myapp/app/views/layouts/explore.html.haml',
base_label: '_app_views_layouts_explore_html_haml',
)
)
end

context 'when filename ends with .rb' do
let(:filename) { 'example.rb' }

it 'does not trim the method name' do
expect(gathered_stack).to eq reference_stack
end
end

context 'when method_name does not end with __number_number' do
let(:method_name) { super().gsub('__', '_') }

it 'does not trim the method name' do
expect(gathered_stack).to eq reference_stack
end
end

context 'when method only has __number_number' do
let(:method_name) { '__2304485752546535910_211320' }

it 'does not trim the method name' do
expect(gathered_stack).to eq reference_stack
end
end
end
end

context 'when sampling a thread with a stack that is deeper than the configured max_frames' do
Expand Down
Loading