From 778e015beaadf4715692edd7d1611fbb940c4533 Mon Sep 17 00:00:00 2001
From: schin1 <schin@gitlab.com>
Date: Thu, 8 Dec 2022 15:09:22 +0800
Subject: [PATCH] Add `redis_allowed_cross_slot_count` into logs
Counting number of allowed cross-slot commands is a cheap way to
approximate the amount of cross-slot violations. The probability of an
allowed cross-slot command is ~1 as long as multiple keys are used.
---
doc/administration/logs/index.md | 2 ++
lib/gitlab/instrumentation/redis.rb | 3 ++-
lib/gitlab/instrumentation/redis_base.rb | 15 +++++++++++++++
lib/gitlab/instrumentation/redis_payload.rb | 1 +
.../gitlab/instrumentation/redis_base_spec.rb | 19 +++++++++++++++++++
.../instrumentation/redis_interceptor_spec.rb | 3 +++
spec/lib/gitlab/instrumentation/redis_spec.rb | 10 ++++++++--
.../lib/gitlab/instrumentation_helper_spec.rb | 9 +++++++--
8 files changed, 57 insertions(+), 5 deletions(-)
diff --git a/doc/administration/logs/index.md b/doc/administration/logs/index.md
index efe91e944b725..9893c6935a352 100644
--- a/doc/administration/logs/index.md
+++ b/doc/administration/logs/index.md
@@ -161,11 +161,13 @@ seconds:
- `gitaly_calls`: Total number of calls made to Gitaly
- `redis_calls`: Total number of calls made to Redis
- `redis_cross_slot_calls`: Total number of cross-slot calls made to Redis
+- `redis_allowed_cross_slot_calls`: Total number of allowed cross-slot calls made to Redis
- `redis_duration_s`: Total time to retrieve data from Redis
- `redis_read_bytes`: Total bytes read from Redis
- `redis_write_bytes`: Total bytes written to Redis
- `redis_<instance>_calls`: Total number of calls made to a Redis instance
- `redis_<instance>_cross_slot_calls`: Total number of cross-slot calls made to a Redis instance
+- `redis_<instance>_allowed_cross_slot_calls`: Total number of allowed cross-slot calls made to a Redis instance
- `redis_<instance>_duration_s`: Total time to retrieve data from a Redis instance
- `redis_<instance>_read_bytes`: Total bytes read from a Redis instance
- `redis_<instance>_write_bytes`: Total bytes written to a Redis instance
diff --git a/lib/gitlab/instrumentation/redis.rb b/lib/gitlab/instrumentation/redis.rb
index da0061a4d9924..a664656c467bf 100644
--- a/lib/gitlab/instrumentation/redis.rb
+++ b/lib/gitlab/instrumentation/redis.rb
@@ -39,7 +39,8 @@ def detail_store
end
end
- %i[get_request_count get_cross_slot_request_count query_time read_bytes write_bytes].each do |method|
+ %i[get_request_count get_cross_slot_request_count get_allowed_cross_slot_request_count query_time read_bytes
+ write_bytes].each do |method|
define_method method do
STORAGES.sum(&method)
end
diff --git a/lib/gitlab/instrumentation/redis_base.rb b/lib/gitlab/instrumentation/redis_base.rb
index f88181820d8bf..de24132a28e11 100644
--- a/lib/gitlab/instrumentation/redis_base.rb
+++ b/lib/gitlab/instrumentation/redis_base.rb
@@ -50,6 +50,11 @@ def increment_cross_slot_request_count(amount = 1)
::RequestStore[cross_slots_key] += amount
end
+ def increment_allowed_cross_slot_request_count(amount = 1)
+ ::RequestStore[allowed_cross_slots_key] ||= 0
+ ::RequestStore[allowed_cross_slots_key] += amount
+ end
+
def get_request_count
::RequestStore[request_count_key] || 0
end
@@ -70,6 +75,10 @@ def get_cross_slot_request_count
::RequestStore[cross_slots_key] || 0
end
+ def get_allowed_cross_slot_request_count
+ ::RequestStore[allowed_cross_slots_key] || 0
+ end
+
def query_time
query_time = ::RequestStore[call_duration_key] || 0
query_time.round(::Gitlab::InstrumentationHelper::DURATION_PRECISION)
@@ -85,6 +94,8 @@ def redis_cluster_validate!(commands)
raise RedisClusterValidator::CrossSlotError, "Redis command #{result[:command_name]} arguments hash to different slots. See https://docs.gitlab.com/ee/development/redis.html#multi-key-commands"
end
+ increment_allowed_cross_slot_request_count if result[:allowed]
+
result[:valid]
end
@@ -144,6 +155,10 @@ def cross_slots_key
strong_memoize(:cross_slots_key) { build_key(:redis_cross_slot_request_count) }
end
+ def allowed_cross_slots_key
+ strong_memoize(:allowed_cross_slots_key) { build_key(:redis_allowed_cross_slot_request_count) }
+ end
+
def build_key(namespace)
"#{storage_key}_#{namespace}"
end
diff --git a/lib/gitlab/instrumentation/redis_payload.rb b/lib/gitlab/instrumentation/redis_payload.rb
index 3a9835410af8d..62a4d1a846f79 100644
--- a/lib/gitlab/instrumentation/redis_payload.rb
+++ b/lib/gitlab/instrumentation/redis_payload.rb
@@ -21,6 +21,7 @@ def to_lazy_payload
{
"#{key_prefix}_calls": -> { get_request_count },
"#{key_prefix}_cross_slot_calls": -> { get_cross_slot_request_count },
+ "#{key_prefix}_allowed_cross_slot_calls": -> { get_allowed_cross_slot_request_count },
"#{key_prefix}_duration_s": -> { query_time },
"#{key_prefix}_read_bytes": -> { read_bytes },
"#{key_prefix}_write_bytes": -> { write_bytes }
diff --git a/spec/lib/gitlab/instrumentation/redis_base_spec.rb b/spec/lib/gitlab/instrumentation/redis_base_spec.rb
index 9cf65a11bce85..656e6ffba058f 100644
--- a/spec/lib/gitlab/instrumentation/redis_base_spec.rb
+++ b/spec/lib/gitlab/instrumentation/redis_base_spec.rb
@@ -109,6 +109,25 @@
end
end
+ describe '.increment_allowed_cross_slot_request_count' do
+ context 'storage key overlapping' do
+ it 'keys do not overlap across storages' do
+ 3.times { instrumentation_class_a.increment_allowed_cross_slot_request_count }
+ 2.times { instrumentation_class_b.increment_allowed_cross_slot_request_count }
+
+ expect(instrumentation_class_a.get_allowed_cross_slot_request_count).to eq(3)
+ expect(instrumentation_class_b.get_allowed_cross_slot_request_count).to eq(2)
+ end
+
+ it 'increments by the given amount' do
+ instrumentation_class_a.increment_allowed_cross_slot_request_count(2)
+ instrumentation_class_a.increment_allowed_cross_slot_request_count(3)
+
+ expect(instrumentation_class_a.get_allowed_cross_slot_request_count).to eq(5)
+ end
+ end
+ end
+
describe '.increment_read_bytes' do
context 'storage key overlapping' do
it 'keys do not overlap across storages' do
diff --git a/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb b/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb
index 120430accf1fb..187a6ff17399d 100644
--- a/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb
+++ b/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb
@@ -83,12 +83,14 @@
it 'counts disallowed cross-slot requests' do
expect(instrumentation_class).to receive(:increment_cross_slot_request_count).and_call_original
+ expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
Gitlab::Redis::SharedState.with { |redis| redis.call(:mget, 'foo', 'bar') }
end
it 'does not count allowed cross-slot requests' do
expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original
+ expect(instrumentation_class).to receive(:increment_allowed_cross_slot_request_count).and_call_original
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
Gitlab::Redis::SharedState.with { |redis| redis.call(:mget, 'foo', 'bar') }
@@ -97,6 +99,7 @@
it 'skips count for non-cross-slot requests' do
expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original
+ expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
Gitlab::Redis::SharedState.with { |redis| redis.call(:mget, '{foo}bar', '{foo}baz') }
end
diff --git a/spec/lib/gitlab/instrumentation/redis_spec.rb b/spec/lib/gitlab/instrumentation/redis_spec.rb
index 7b6f31c1a0802..3e02eadba4b7d 100644
--- a/spec/lib/gitlab/instrumentation/redis_spec.rb
+++ b/spec/lib/gitlab/instrumentation/redis_spec.rb
@@ -24,6 +24,7 @@ def stub_storages(method, value)
it_behaves_like 'aggregation of redis storage data', :get_request_count
it_behaves_like 'aggregation of redis storage data', :get_cross_slot_request_count
+ it_behaves_like 'aggregation of redis storage data', :get_allowed_cross_slot_request_count
it_behaves_like 'aggregation of redis storage data', :query_time
it_behaves_like 'aggregation of redis storage data', :read_bytes
it_behaves_like 'aggregation of redis storage data', :write_bytes
@@ -39,21 +40,26 @@ def stub_storages(method, value)
stub_rails_env('staging') # to avoid raising CrossSlotError
Gitlab::Redis::Cache.with { |redis| redis.mset('cache-test', 321, 'cache-test-2', 321) }
+ Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
+ Gitlab::Redis::Cache.with { |redis| redis.mget('cache-test', 'cache-test-2') }
+ end
Gitlab::Redis::SharedState.with { |redis| redis.set('shared-state-test', 123) }
end
it 'returns payload filtering out zeroed values' do
expected_payload = {
# Aggregated results
- redis_calls: 2,
+ redis_calls: 3,
redis_cross_slot_calls: 1,
+ redis_allowed_cross_slot_calls: 1,
redis_duration_s: be >= 0,
redis_read_bytes: be >= 0,
redis_write_bytes: be >= 0,
# Cache results
- redis_cache_calls: 1,
+ redis_cache_calls: 2,
redis_cache_cross_slot_calls: 1,
+ redis_cache_allowed_cross_slot_calls: 1,
redis_cache_duration_s: be >= 0,
redis_cache_read_bytes: be >= 0,
redis_cache_write_bytes: be >= 0,
diff --git a/spec/lib/gitlab/instrumentation_helper_spec.rb b/spec/lib/gitlab/instrumentation_helper_spec.rb
index b4738ed663c9f..7d78d25f18e79 100644
--- a/spec/lib/gitlab/instrumentation_helper_spec.rb
+++ b/spec/lib/gitlab/instrumentation_helper_spec.rb
@@ -41,13 +41,17 @@
it 'adds Redis data and omits Gitaly data' do
stub_rails_env('staging') # to avoid raising CrossSlotError
Gitlab::Redis::Cache.with { |redis| redis.mset('test-cache', 123, 'test-cache2', 123) }
+ Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
+ Gitlab::Redis::Cache.with { |redis| redis.mget('cache-test', 'cache-test-2') }
+ end
Gitlab::Redis::Queues.with { |redis| redis.set('test-queues', 321) }
subject
# Aggregated payload
- expect(payload[:redis_calls]).to eq(2)
+ expect(payload[:redis_calls]).to eq(3)
expect(payload[:redis_cross_slot_calls]).to eq(1)
+ expect(payload[:redis_allowed_cross_slot_calls]).to eq(1)
expect(payload[:redis_duration_s]).to be >= 0
expect(payload[:redis_read_bytes]).to be >= 0
expect(payload[:redis_write_bytes]).to be >= 0
@@ -59,8 +63,9 @@
expect(payload[:redis_queues_write_bytes]).to be >= 0
# Cache payload
- expect(payload[:redis_cache_calls]).to eq(1)
+ expect(payload[:redis_cache_calls]).to eq(2)
expect(payload[:redis_cache_cross_slot_calls]).to eq(1)
+ expect(payload[:redis_cache_allowed_cross_slot_calls]).to eq(1)
expect(payload[:redis_cache_duration_s]).to be >= 0
expect(payload[:redis_cache_read_bytes]).to be >= 0
expect(payload[:redis_cache_write_bytes]).to be >= 0
--
GitLab