[PROF-10112] Simplify method names from actionview templates in profiler #3759
Conversation
**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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3759 +/- ##
==========================================
- Coverage 97.91% 97.91% -0.01%
==========================================
Files 1241 1241
Lines 74632 74654 +22
Branches 3605 3605
==========================================
+ Hits 73074 73095 +21
- Misses 1558 1559 +1 ☔ View full report in Codecov by Sentry. |
This is a good point! I double-checked and the answer is actually that the prefix is the path to the template, .e.g And because it's a path, it's actually possible to change it. By using So while I don't think it'll be very common that the template will come from outside the I also don't think it would particularly be faster -- since we already have the file name filter (not ".rb") + most methods don't end with numbers, this should be enough to filter out almost all methods which are not templates. |
|
I've gone ahead and merged the PR, but LMK if you think I should give another look at focusing only on the |
|
My concern was that if someone has their methods named with numeric suffixes that happen to match Rails' generated method names that this PR is considering, they would have their methods unexpectedly renamed. But I wouldn't expect such naming to be very common either. |
Yeah, they'd need to be quite unlucky:
Hopefully the current version is good enough; if someone ever triggers this by accident we'll have examples on how to tighten the pattern to not match the accidental one as well :) |
**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.
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
erborhamlwe'll be able to validate they no longer end with the ids.