From 7d81345f966666034c3d6fc6bf5da9ca79484be1 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Fri, 5 Jul 2024 09:05:59 +0100 Subject: [PATCH] [10112] Simplify method names from actionview templates in profiler **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. --- .../collectors_stack.c | 32 ++++++++++ .../profiling/collectors/stack_spec.rb | 61 +++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/ext/datadog_profiling_native_extension/collectors_stack.c b/ext/datadog_profiling_native_extension/collectors_stack.c index 7cb073928e9..e9bb93643f4 100644 --- a/ext/datadog_profiling_native_extension/collectors_stack.c +++ b/ext/datadog_profiling_native_extension/collectors_stack.c @@ -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"); @@ -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. @@ -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; diff --git a/spec/datadog/profiling/collectors/stack_spec.rb b/spec/datadog/profiling/collectors/stack_spec.rb index 80d1e8c0d94..6cedb85dbe1 100644 --- a/spec/datadog/profiling/collectors/stack_spec.rb +++ b/spec/datadog/profiling/collectors/stack_spec.rb @@ -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