Skip to content

Commit

Permalink
[PROF-10112] Simplify method names from actionview templates in profiler
Browse files Browse the repository at this point in the history
**What does this PR do?**

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.

This PR modifies the stack collector to detect such methods and
simplify them by slicing the hash/id suffix.

**Motivation:**

Improve quality of aggregated flamegraph.

**Additional Notes:**

N/A

**How to test the change?**

Change includes test coverage. Also, all of our internal Rails test
apps use templates, so by searching by methods with `erb` or
`haml` we'll be able to validate they no longer end with the ids.
  • Loading branch information
ivoanjo committed Jul 5, 2024
1 parent 849c60c commit 5c6441a
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 0 deletions.
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

0 comments on commit 5c6441a

Please sign in to comment.