PERF: Improve snapshots page performance (#518)

Currently, the time that the snapshots page (`/mini-profiler-resources/snapshots`) takes to load grows linearly as the number of snapshots grows. By default we limit the number of snapshots to 1000, which is a reasonable limit, but when we reach 1000 snapshots the performance gets really bad that the page takes 4 seconds to load when using the Redis storage backend (everything said here applies to the Redis backend).

The reason for the bad performance is how snapshots are stored -- we keep all snapshots in a single redis hash where snapshot IDs map to marshalled snapshot objects, and when the snapshots page is requested we have to fetch everything from that redis hash and convert the data back to ruby objects to be able to group them and sort them etc. This means that we need to spend a lot time doing IO to transfer all the snapshots data from the redis server to the web server. A snapshot is on average ~280 KBs and since we allow 1000 snapshots by default, we have to fetch ~280 MBs worth of data everytime the snapshots page is requested.

To fix this performance problem, this commit changes the storage model so that every snapshot group is stored separately in its own redis hash, and we keep track of all group names and their worst score in an "overview" redis sorted set. This storage model allows us to respond to the snapshots page without having to fetch all the snapshots data; we can simply read the overview redis sorted set which is very small.

The downside of this new approach/storage model is that we have to do a little more work when saving a new snapshot. Specifically, we have to update the group's score in the overview set if necessary when a new snapshot is added to a group. We also have new limits on the number of groups and the number of snapshots per group that we need to respect when saving a new snapshot. That said, this extra overhead should be negligible and I think the tradeoff is completely worth it.

Note that this change doesn't migrate existing snapshots to the new storage scheme, so when this change is deployed the old snapshots won't show up in the snapshots page but they'll still be retained in redis.

Author: OsamaSayegh

README.md

@@ -207,7 +207,7 @@ After enabling snapshots sampling, you can see the snapshots that have been coll
 
 Access to the snapshots page is restricted to only those who can see the speed badge on their own requests, see the section below this one about access control.
 
-Mini Profiler will keep a maximum of 1000 snapshots by default, and you can change that via the `snapshots_limit` config. When snapshots reach the configured limit, Mini Profiler will save a new snapshot only if it's worse than at least one of the existing snapshots and delete the best one (i.e. the snapshot whose request took the least time compared to other snapshots).
+Mini Profiler will keep a maximum of 50 snapshot groups and a maximum of 15 snapshots per group making the default maximum number of snapshots in the system 750. The default group and per group limits can be changed via the `max_snapshot_groups` and `max_snapshots_per_group` configuration options, see the configurations table below.
 
 #### Snapshots Transporter
 
@@ -430,7 +430,8 @@ show_total_sql_count|`false`|Displays the total number of SQL executions.
 enable_advanced_debugging_tools|`false`|Enables sensitive debugging tools that can be used via the UI. In production we recommend keeping this disabled as memory and environment debugging tools can expose contents of memory that may contain passwords. Defaults to `true` in development.
 assets_url|`nil`|See the "Register MiniProfiler's assets in the Rails assets pipeline" section above.
 snapshot_every_n_requests|`-1`|Determines how frequently snapshots are taken. See the "Snapshots Sampling" above for more details.
-snapshots_limit|`1000`|Determines how many snapshots Mini Profiler is allowed to keep.
+max_snapshot_groups|`50`|Determines how many snapshot groups Mini Profiler is allowed to keep.
+max_snapshots_per_group|`15`|Determines how many snapshots per group Mini Profiler is allowed to keep.
 snapshot_hidden_custom_fields|`[]`|Each snapshot custom field will have a dedicated column in the UI by default. Use this config to exclude certain custom fields from having their own columns.
 snapshots_transport_destination_url|`nil`|Set this config to a valid URL to enable snapshots transporter which will `POST` snapshots to the given URL. The transporter requires `snapshots_transport_auth_key` config to be set as well.
 snapshots_transport_auth_key|`nil`|`POST` requests made by the snapshots transporter to the destination URL will have a `Mini-Profiler-Transport-Auth` header with the value of this config. Make sure you use a secure and random key for this config.

lib/mini_profiler/config.rb

@@ -40,7 +40,8 @@ def self.default
           @skip_sql_param_names = /password/ # skips parameters with the name password by default
           @enable_advanced_debugging_tools = false
           @snapshot_every_n_requests = -1
-          @snapshots_limit = 1000
+          @max_snapshot_groups = 50
+          @max_snapshots_per_group = 15
 
           # ui parameters
           @autorized            = true
@@ -81,10 +82,10 @@ def self.default
         :start_hidden, :toggle_shortcut, :html_container
 
       # snapshot related config
-      attr_accessor :snapshot_every_n_requests, :snapshots_limit,
+      attr_accessor :snapshot_every_n_requests, :max_snapshots_per_group,
         :snapshot_hidden_custom_fields, :snapshots_transport_destination_url,
         :snapshots_transport_auth_key, :snapshots_redact_sql_queries,
-        :snapshots_transport_gzip_requests
+        :snapshots_transport_gzip_requests, :max_snapshot_groups
 
       # Deprecated options
       attr_accessor :use_existing_jquery

lib/mini_profiler/profiler.rb

@@ -130,10 +130,10 @@ def user(env)
     def serve_results(env)
       request     = Rack::Request.new(env)
       id          = request.params['id']
-      is_snapshot = request.params['snapshot']
-      is_snapshot = [true, "true"].include?(is_snapshot)
+      group_name  = request.params['group']
+      is_snapshot = group_name && group_name.size > 0
       if is_snapshot
-        page_struct = @storage.load_snapshot(id)
+        page_struct = @storage.load_snapshot(id, group_name)
       else
         page_struct = @storage.load(id)
       end
@@ -802,16 +802,16 @@ def handle_snapshots_request(env)
       headers = { 'Content-Type' => 'text/html' }
       qp = Rack::Utils.parse_nested_query(env['QUERY_STRING'])
       if group_name = qp["group_name"]
-        list = @storage.find_snapshots_group(group_name)
+        list = @storage.snapshots_group(group_name)
         list.each do |snapshot|
-          snapshot[:url] = url_for_snapshot(snapshot[:id])
+          snapshot[:url] = url_for_snapshot(snapshot[:id], group_name)
         end
         data = {
           group_name: group_name,
           list: list
         }
       else
-        list = @storage.snapshot_groups_overview
+        list = @storage.snapshots_overview
         list.each do |group|
           group[:url] = url_for_snapshots_group(group[:name])
         end
@@ -864,7 +864,7 @@ def rails_route_from_path(path, method)
       if defined?(Rails) && defined?(ActionController::RoutingError)
         hash = Rails.application.routes.recognize_path(path, method: method)
         if hash && hash[:controller] && hash[:action]
-          "#{method} #{hash[:controller]}##{hash[:action]}"
+          "#{hash[:controller]}##{hash[:action]}"
         end
       end
     rescue ActionController::RoutingError
@@ -876,8 +876,8 @@ def url_for_snapshots_group(group_name)
       "/#{@config.base_url_path.gsub('/', '')}/snapshots?#{qs}"
     end
 
-    def url_for_snapshot(id)
-      qs = Rack::Utils.build_query({ id: id, snapshot: true })
+    def url_for_snapshot(id, group_name)
+      qs = Rack::Utils.build_query({ id: id, group: group_name })
       "/#{@config.base_url_path.gsub('/', '')}/results?#{qs}"
     end
 
@@ -902,8 +902,12 @@ def take_snapshot(env, start)
         if Rack::MiniProfiler.snapshots_transporter?
           Rack::MiniProfiler::SnapshotsTransporter.transport(page_struct)
         else
+          group_name = rails_route_from_path(page_struct[:request_path], page_struct[:request_method])
+          group_name ||= page_struct[:request_path]
+          group_name = "#{page_struct[:request_method]} #{group_name}"
           @storage.push_snapshot(
             page_struct,
+            group_name,
             @config
           )
         end

lib/mini_profiler/storage/abstract_store.rb

@@ -45,80 +45,53 @@ def should_take_snapshot?(period)
         raise NotImplementedError.new("should_take_snapshot? is not implemented")
       end
 
-      def push_snapshot(page_struct, config)
+      def push_snapshot(page_struct, group_name, config)
         raise NotImplementedError.new("push_snapshot is not implemented")
       end
 
-      def fetch_snapshots(batch_size: 200, &blk)
-        raise NotImplementedError.new("fetch_snapshots is not implemented")
+      # returns a hash where the keys are group names and the values
+      # are hashes that contain 3 keys:
+      #   1. `:worst_score` => the duration of the worst/slowest snapshot in the group (float)
+      #   2. `:best_score` => the duration of the best/fastest snapshot in the group (float)
+      #   3. `:snapshots_count` => the number of snapshots in the group (integer)
+      def fetch_snapshots_overview
+        raise NotImplementedError.new("fetch_snapshots_overview is not implemented")
       end
 
-      def snapshot_groups_overview
-        groups = {}
-        fetch_snapshots do |batch|
-          batch.each do |snapshot|
-            group_name = default_snapshot_grouping(snapshot)
-            hash = groups[group_name] ||= {}
-            hash[:snapshots_count] ||= 0
-            hash[:snapshots_count] += 1
-            if !hash[:worst_score] || hash[:worst_score] < snapshot.duration_ms
-              groups[group_name][:worst_score] = snapshot.duration_ms
-            end
-            if !hash[:best_score] || hash[:best_score] > snapshot.duration_ms
-              groups[group_name][:best_score] = snapshot.duration_ms
-            end
-          end
-        end
-        groups = groups.to_a
+      # @param group_name [String]
+      # @return [Array<Rack::MiniProfiler::TimerStruct::Page>] list of snapshots of the group. Blank array if the group doesn't exist.
+      def fetch_snapshots_group(group_name)
+        raise NotImplementedError.new("fetch_snapshots_group is not implemented")
+      end
+
+      def load_snapshot(id, group_name)
+        raise NotImplementedError.new("load_snapshot is not implemented")
+      end
+
+      def snapshots_overview
+        groups = fetch_snapshots_overview.to_a
         groups.sort_by! { |name, hash| hash[:worst_score] }
         groups.reverse!
         groups.map! { |name, hash| hash.merge(name: name) }
         groups
       end
 
-      def find_snapshots_group(group_name)
+      def snapshots_group(group_name)
+        snapshots = fetch_snapshots_group(group_name)
         data = []
-        fetch_snapshots do |batch|
-          batch.each do |snapshot|
-            snapshot_group_name = default_snapshot_grouping(snapshot)
-            if group_name == snapshot_group_name
-              data << {
-                id: snapshot[:id],
-                duration: snapshot.duration_ms,
-                sql_count: snapshot[:sql_count],
-                timestamp: snapshot[:started_at],
-                custom_fields: snapshot[:custom_fields]
-              }
-            end
-          end
+        snapshots.each do |snapshot|
+          data << {
+            id: snapshot[:id],
+            duration: snapshot.duration_ms,
+            sql_count: snapshot[:sql_count],
+            timestamp: snapshot[:started_at],
+            custom_fields: snapshot[:custom_fields]
+          }
         end
         data.sort_by! { |s| s[:duration] }
         data.reverse!
         data
       end
-
-      def load_snapshot(id)
-        raise NotImplementedError.new("load_snapshot is not implemented")
-      end
-
-      private
-
-      def default_snapshot_grouping(snapshot)
-        group_name = rails_route_from_path(snapshot[:request_path], snapshot[:request_method])
-        group_name ||= snapshot[:request_path]
-        "#{snapshot[:request_method]} #{group_name}"
-      end
-
-      def rails_route_from_path(path, method)
-        if defined?(Rails) && defined?(ActionController::RoutingError)
-          hash = Rails.application.routes.recognize_path(path, method: method)
-          if hash && hash[:controller] && hash[:action]
-            "#{hash[:controller]}##{hash[:action]}"
-          end
-        end
-      rescue ActionController::RoutingError
-        nil
-      end
     end
   end
 end

lib/mini_profiler/storage/memory_store.rb

@@ -53,6 +53,7 @@ def initialize(args = nil)
 
         @token1, @token2, @cycle_at = nil
         @snapshots_cycle = 0
+        @snapshot_groups = {}
         @snapshots = []
 
         initialize_locks
@@ -152,28 +153,69 @@ def should_take_snapshot?(period)
         end
       end
 
-      def push_snapshot(page_struct, config)
+      def push_snapshot(page_struct, group_name, config)
         @snapshots_lock.synchronize do
-          @snapshots << page_struct
-          @snapshots.sort_by! { |s| s.duration_ms }
-          @snapshots.reverse!
-          if @snapshots.size > config.snapshots_limit
-            @snapshots.slice!(-1)
+          group = @snapshot_groups[group_name]
+          if !group
+            @snapshot_groups[group_name] = {
+              worst_score: page_struct.duration_ms,
+              best_score: page_struct.duration_ms,
+              snapshots: [page_struct]
+            }
+            if @snapshot_groups.size > config.max_snapshot_groups
+              group_keys = @snapshot_groups.keys
+              group_keys.sort_by! do |key|
+                @snapshot_groups[key][:worst_score]
+              end
+              group_keys.reverse!
+              group_keys.pop(group_keys.size - config.max_snapshot_groups)
+              @snapshot_groups = @snapshot_groups.slice(*group_keys)
+            end
+          else
+            snapshots = group[:snapshots]
+            snapshots << page_struct
+            snapshots.sort_by!(&:duration_ms)
+            snapshots.reverse!
+            if snapshots.size > config.max_snapshots_per_group
+              snapshots.pop(snapshots.size - config.max_snapshots_per_group)
+            end
+            group[:worst_score] = snapshots[0].duration_ms
+            group[:best_score] = snapshots[-1].duration_ms
           end
         end
       end
 
-      def fetch_snapshots(batch_size: 200, &blk)
+      def fetch_snapshots_overview
         @snapshots_lock.synchronize do
-          @snapshots.each_slice(batch_size) do |batch|
-            blk.call(batch)
+          groups = {}
+          @snapshot_groups.each do |name, group|
+            groups[name] = {
+              worst_score: group[:worst_score],
+              best_score: group[:best_score],
+              snapshots_count: group[:snapshots].size
+            }
           end
+          groups
         end
       end
 
-      def load_snapshot(id)
+      def fetch_snapshots_group(group_name)
         @snapshots_lock.synchronize do
-          @snapshots.find { |s| s[:id] == id }
+          group = @snapshot_groups[group_name]
+          if group
+            group[:snapshots].dup
+          else
+            []
+          end
+        end
+      end
+
+      def load_snapshot(id, group_name)
+        @snapshots_lock.synchronize do
+          group = @snapshot_groups[group_name]
+          if group
+            group[:snapshots].find { |s| s[:id] == id }
+          end
         end
       end
 
@@ -182,7 +224,7 @@ def load_snapshot(id)
       # used in tests only
       def wipe_snapshots_data
         @snapshots_cycle = 0
-        @snapshots = []
+        @snapshot_groups = {}
       end
     end
   end