Merge branch 'sh-fix-redis-performance-bar' into 'master'

Fix inconsistency in Redis performance bar stats

Closes #64707

See merge request gitlab-org/gitlab-ce!30866

Gemfile

@@ -299,7 +299,6 @@ gem 'peek-gc', '~> 0.0.2'
 gem 'peek-mysql2', '~> 1.2.0', group: :mysql
 gem 'peek-pg', '~> 1.3.0', group: :postgres
 gem 'peek-rblineprof', '~> 0.2.0'
-gem 'peek-redis', '~> 1.2.0'
 
 # Memory benchmarks
 gem 'derailed_benchmarks', require: false

Gemfile.lock

@@ -76,7 +76,6 @@ GEM
     asciidoctor-plantuml (0.0.9)
       asciidoctor (>= 1.5.6, < 3.0.0)
     ast (2.4.0)
-    atomic (1.1.99)
     attr_encrypted (3.1.0)
       encryptor (~> 3.0.0)
     attr_required (1.0.1)
@@ -658,10 +657,6 @@ GEM
     peek-rblineprof (0.2.0)
       peek
       rblineprof
-    peek-redis (1.2.0)
-      atomic (>= 1.0.0)
-      peek
-      redis
     pg (1.1.4)
     po_to_json (1.0.1)
       json (>= 1.6.0)
@@ -1199,7 +1194,6 @@ DEPENDENCIES
   peek-mysql2 (~> 1.2.0)
   peek-pg (~> 1.3.0)
   peek-rblineprof (~> 0.2.0)
-  peek-redis (~> 1.2.0)
   pg (~> 1.1)
   premailer-rails (~> 1.9.7)
   prometheus-client-mmap (~> 0.9.8)

config/initializers/peek.rb

@@ -29,7 +29,7 @@ end
 Peek.into PEEK_DB_VIEW
 Peek.into Peek::Views::Gitaly
 Peek.into Peek::Views::Rblineprof
-Peek.into Peek::Views::Redis
+Peek.into Peek::Views::RedisDetailed
 Peek.into Peek::Views::GC
 Peek.into Peek::Views::Tracing if Labkit::Tracing.tracing_url_enabled?
 

lib/peek/views/redis.rb → lib/peek/views/redis_detailed.rb

 # frozen_string_literal: true
 
 require 'redis'
-require 'peek-redis'
 
 module Gitlab
   module Peek
@@ -36,11 +35,42 @@ end
 
 module Peek
   module Views
-    module RedisDetailed
+    class RedisDetailed < View
       REDACTED_MARKER = "<redacted>"
 
+      def key
+        'redis'
+      end
+
       def results
-        super.merge(details: details)
+        {
+          calls: calls,
+          duration: formatted_duration,
+          details: details
+        }
+      end
+
+      def detail_store
+        ::Gitlab::SafeRequestStore['redis_call_details'] ||= []
+      end
+
+      private
+
+      def formatted_duration
+        ms = duration * 1000
+        if ms >= 1000
+          "%.2fms" % ms
+        else
+          "%.0fms" % ms
+        end
+      end
+
+      def duration
+        detail_store.map { |entry| entry[:duration] }.sum # rubocop:disable CodeReuse/ActiveRecord
+      end
+
+      def calls
+        detail_store.count
       end
 
       def details
@@ -49,10 +79,6 @@ module Peek
           .map(&method(:format_call_details))
       end
 
-      def detail_store
-        ::Gitlab::SafeRequestStore['redis_call_details'] ||= []
-      end
-
       def format_call_details(call)
         call.merge(cmd: format_command(call[:cmd]),
                    duration: (call[:duration] * 1000).round(3))
@@ -76,11 +102,3 @@ end
 class Redis::Client
   prepend Gitlab::Peek::RedisInstrumented
 end
-
-module Peek
-  module Views
-    class Redis < View
-      prepend Peek::Views::RedisDetailed
-    end
-  end
-end

spec/lib/peek/views/redis_detailed_spec.rb

@@ -2,14 +2,8 @@
 
 require 'spec_helper'
 
-describe Peek::Views::RedisDetailed do
-  let(:redis_detailed_class) do
-    Class.new do
-      include Peek::Views::RedisDetailed
-    end
-  end
-
-  subject { redis_detailed_class.new }
+describe Peek::Views::RedisDetailed, :request_store do
+  subject { described_class.new }
 
   using RSpec::Parameterized::TableSyntax
 
@@ -22,15 +16,24 @@ describe Peek::Views::RedisDetailed do
   end
 
   with_them do
-    it 'scrubs Redis commands', :request_store do
+    it 'scrubs Redis commands' do
       subject.detail_store << { cmd: cmd, duration: 1.second }
 
-      expect(subject.details.count).to eq(1)
-      expect(subject.details.first)
+      expect(subject.results[:details].count).to eq(1)
+      expect(subject.results[:details].first)
         .to eq({
                  cmd: expected,
                  duration: 1000
                })
     end
   end
+
+  it 'returns aggregated results' do
+    subject.detail_store << { cmd: [:get, 'test'], duration: 0.001 }
+    subject.detail_store << { cmd: [:get, 'test'], duration: 1.second }
+
+    expect(subject.results[:calls]).to eq(2)
+    expect(subject.results[:duration]).to eq('1001.00ms')
+    expect(subject.results[:details].count).to eq(2)
+  end
 end