From 52f6cbd67e6c52c5e640fad27f8c0933eac3659a Mon Sep 17 00:00:00 2001
From: Ivo Anjo <ivo.anjo@datadoghq.com>
Date: Wed, 10 Jul 2024 09:21:52 +0100
Subject: [PATCH] [PROF-10112] Also simplify actionview templates with three
 underscores

**What does this PR do?**

This PR is a small fix on top of #3759. In that PR, we added code to
detect method names dynamically generated by Rails actionview templates,
slicing the parts of the method name that have random ids.

E.g. `_app_views_layouts_explore_html_haml__2304485752546535910_211320`
became `_app_views_layouts_explore_html_haml`.

When looking at one of our example applications, I realized that I
was seeing methods that ended with both `__number_number` as well as
`___number_number` (three vs two underscores), e.g.:

* `_app_views_articles_index_html_erb___2022809201779434309_12900`
* `_app_views_articles_index_html_erb__3816154855874711207_12920`

On closer inspection of the actionview template naming code in
https://github.com/rails/rails/blob/4fa56814f18fd3da49c83931fa773caa727d8096/actionview/lib/action_view/template.rb#L389:

```ruby
    def method_name # :nodoc:
      @method_name ||= begin
        m = +"_#{identifier_method_name}__#{@identifier.hash}_#{__id__}"
        m.tr!("-", "_")
        m
      end
    end
```

... I realized that when `@identifier.hash` was a negative number,
it would be printed including the - sign, which would be replaced by
the `tr` into another `_`. (It's somewhat weird that they didn't just go
with `hash.abs` but yeah... >_>).

Thus, there was a 50-50 chance that methods end up with three
underscores, which would make them not merge together in the flamegraph.

This PR fixes this.

**Motivation:**

Make sure that these dynamically-generated methods merge together
properly in a flamegraph.

**Additional Notes:**

N/A

**How to test the change?**

This change includes test coverage.
---
 .../collectors_stack.c                            | 12 ++++++++++--
 spec/datadog/profiling/collectors/stack_spec.rb   | 15 ++++++++++++++-
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/ext/datadog_profiling_native_extension/collectors_stack.c b/ext/datadog_profiling_native_extension/collectors_stack.c
index 28d7988cc61..fd50f6a591b 100644
--- a/ext/datadog_profiling_native_extension/collectors_stack.c
+++ b/ext/datadog_profiling_native_extension/collectors_stack.c
@@ -274,9 +274,14 @@ 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".
+// names such as:
+// * "_app_views_layouts_explore_html_haml__2304485752546535910_211320" (__number_number suffix -- two underscores)
+// * "_app_views_articles_index_html_erb___2022809201779434309_12900" (___number_number suffix -- three underscores)
 // 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 )
+// (Reference:
+//  https://github.com/rails/rails/blob/4fa56814f18fd3da49c83931fa773caa727d8096/actionview/lib/action_view/template.rb#L389
+//  The two vs three underscores happen when @identifier.hash is negative in that method: the "-" gets replaced with
+//  the extra "_".)
 //
 // 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) {
@@ -298,6 +303,9 @@ static void maybe_trim_template_random_ids(ddog_CharSlice *name_slice, ddog_Char
   // 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;
 
+  // Does it have the optional third underscore? If so, remove it as well
+  if (pos > 1 && name_slice->ptr[pos-1] == '_') pos--;
+
   // If we got here, we matched on our pattern. Let's slice the length of the string to exclude it.
   name_slice->len = pos;
 }
diff --git a/spec/datadog/profiling/collectors/stack_spec.rb b/spec/datadog/profiling/collectors/stack_spec.rb
index 6cedb85dbe1..7a4e0dc4313 100644
--- a/spec/datadog/profiling/collectors/stack_spec.rb
+++ b/spec/datadog/profiling/collectors/stack_spec.rb
@@ -465,7 +465,7 @@ def dummy_template.#{method_name}(ready_queue)
         # rubocop:enable Style/DocumentDynamicEvalDefinition
       end
 
-      it 'has a frame with a simplified method name' do
+      it 'samples the frame with a simplified method name' do
         expect(gathered_stack).to include(
           have_attributes(
             path: '/myapp/app/views/layouts/explore.html.haml',
@@ -474,6 +474,19 @@ def dummy_template.#{method_name}(ready_queue)
         )
       end
 
+      context 'when method name ends with three ___ instead of two' do
+        let(:method_name) { super().gsub('__', '___') }
+
+        it 'samples the 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
+      end
+
       context 'when filename ends with .rb' do
         let(:filename) { 'example.rb' }