diff options
author | Stan Hu <stanhu@gmail.com> | 2019-07-17 12:33:49 -0700 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2019-07-17 15:11:01 -0700 |
commit | 9dd59df6991b9d82bcbb95bf406194aab8ecf743 (patch) | |
tree | 8e1331802676781b266be121e8e228cb272fa054 | |
parent | c11eb0c3a42dba550764e96426dd9bf21347b917 (diff) | |
download | gitlab-ce-sh-fix-redis-performance-bar.tar.gz |
Fix inconsistency in Redis performance bar statssh-fix-redis-performance-bar
peek-redis resets its counters at the start of an ActionController
notification (`start_processing.action_controller`), which causes it to
miss some Redis queries that precede it, such as the database load
balancer and Rack Attack queries. This produces inconsistencies in the
performance bar between the number of calls and their durations with the
actual calls in the detailed view.
We fix this by getting rid of peek-redis in favor of consolidating all
logic into the `RedisDetailed` view, which tracks Redis queries using
`RequestStore`. This has the nice property of removing thread-specific
counters as well.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64707
-rw-r--r-- | Gemfile | 1 | ||||
-rw-r--r-- | Gemfile.lock | 6 | ||||
-rw-r--r-- | config/initializers/peek.rb | 2 | ||||
-rw-r--r-- | lib/peek/views/redis_detailed.rb (renamed from lib/peek/views/redis.rb) | 48 | ||||
-rw-r--r-- | spec/lib/peek/views/redis_detailed_spec.rb | 25 |
5 files changed, 48 insertions, 34 deletions
@@ -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 diff --git a/Gemfile.lock b/Gemfile.lock index 60939ae918c..2bcc3527de4 100644 --- a/Gemfile.lock +++ b/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) diff --git a/config/initializers/peek.rb b/config/initializers/peek.rb index d492b60705d..2aec32e878c 100644 --- a/config/initializers/peek.rb +++ b/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? diff --git a/lib/peek/views/redis.rb b/lib/peek/views/redis_detailed.rb index 73de8672fa4..12760c9b75e 100644 --- a/lib/peek/views/redis.rb +++ b/lib/peek/views/redis_detailed.rb @@ -1,7 +1,6 @@ # 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 diff --git a/spec/lib/peek/views/redis_detailed_spec.rb b/spec/lib/peek/views/redis_detailed_spec.rb index da13b6df53b..61096e6c69e 100644 --- a/spec/lib/peek/views/redis_detailed_spec.rb +++ b/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 |