diff --git a/instrumentation/redis/Appraisals b/instrumentation/redis/Appraisals index 7939892cfe..abb68fa4be 100644 --- a/instrumentation/redis/Appraisals +++ b/instrumentation/redis/Appraisals @@ -4,3 +4,7 @@ appraise 'redis-4.x' do gem 'redis-client', '~> 0.22' gem 'redis', '~> 4.8' end + +appraise 'redis-5.x' do + gem 'redis', '~> 5.0' +end diff --git a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client.rb b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client.rb index dec9c1c7e2..7599cf3f5b 100644 --- a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client.rb +++ b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client.rb @@ -58,7 +58,7 @@ def serialize_commands(commands) serialized_commands = commands.map do |command| # If we receive an authentication request command we want to obfuscate it - if obfuscate || command[0].match?(/\A(AUTH|HELLO)\z/) + if obfuscate || command[0].match?(/\A(AUTH|HELLO)\z/i) command[0].to_s.upcase + (' ?' * (command.size - 1)) else command_copy = command.dup diff --git a/instrumentation/redis/test/opentelemetry/instrumentation/redis/patches/client_test.rb b/instrumentation/redis/test/opentelemetry/instrumentation/redis/patches/client_test.rb index 96a3cee7d5..7fbf262c27 100644 --- a/instrumentation/redis/test/opentelemetry/instrumentation/redis/patches/client_test.rb +++ b/instrumentation/redis/test/opentelemetry/instrumentation/redis/patches/client_test.rb @@ -10,6 +10,8 @@ require_relative '../../../../../lib/opentelemetry/instrumentation/redis/patches/redis_v4_client' describe OpenTelemetry::Instrumentation::Redis::Patches::RedisV4Client do + # NOTE: These tests should be run for redis v4 and redis v5, even though the patches won't be installed on v5. + # Perhaps these tests should live in a different file? let(:instrumentation) { OpenTelemetry::Instrumentation::Redis::Instrumentation.instance } let(:exporter) { EXPORTER } let(:password) { 'passw0rd' } @@ -21,12 +23,24 @@ # will generate one extra span on connect because the Redis client will # send an AUTH command before doing anything else. def redis_with_auth(redis_options = {}) - redis_options[:password] = password - redis_options[:host] = redis_host - redis_options[:port] = redis_port + redis_options[:password] ||= password + redis_options[:host] ||= redis_host + redis_options[:port] ||= redis_port Redis.new(redis_options) end + def redis_version + Gem.loaded_specs['redis']&.version + end + + def redis_version_major + redis_version&.segments&.first + end + + def redis_gte_5? + redis_version_major&.>=(5) + end + before do # ensure obfuscation is off if it was previously set in a different test config = { db_statement: :include } @@ -98,16 +112,27 @@ def redis_with_auth(redis_options = {}) redis = redis_with_auth(db: 1) redis.get('K') - _(exporter.finished_spans.size).must_equal 3 + if redis_gte_5? + _(exporter.finished_spans.size).must_equal 2 + select_span = exporter.finished_spans.first + get_span = exporter.finished_spans.last + _(select_span.name).must_equal 'PIPELINED' + _(select_span.attributes['db.statement']).must_equal("AUTH ?\nSELECT 1") + else + _(exporter.finished_spans.size).must_equal 3 + + select_span = exporter.finished_spans[1] + _(select_span.name).must_equal 'SELECT' + _(select_span.attributes['db.statement']).must_equal('SELECT 1') + + get_span = exporter.finished_spans.last + end - select_span = exporter.finished_spans[1] - _(select_span.name).must_equal 'SELECT' _(select_span.attributes['db.system']).must_equal 'redis' - _(select_span.attributes['db.statement']).must_equal('SELECT 1') _(select_span.attributes['net.peer.name']).must_equal redis_host _(select_span.attributes['net.peer.port']).must_equal redis_port + _(select_span.attributes['db.redis.database_index']).must_equal 1 - get_span = exporter.finished_spans.last _(get_span.name).must_equal 'GET' _(get_span.attributes['db.system']).must_equal 'redis' _(get_span.attributes['db.statement']).must_equal('GET K') @@ -150,21 +175,35 @@ def redis_with_auth(redis_options = {}) _(last_span.status.code).must_equal( OpenTelemetry::Trace::Status::ERROR ) - _(last_span.status.description.tr('`', "'")).must_include( - "ERR unknown command 'THIS_IS_NOT_A_REDIS_FUNC', with args beginning with: 'THIS_IS_NOT_A_VALID_ARG" - ) + + if redis_gte_5? + _(last_span.status.description.tr('`', "'")).must_include( + 'Unhandled exception of type: RedisClient::CommandError' + ) + else + _(last_span.status.description.tr('`', "'")).must_include( + "ERR unknown command 'THIS_IS_NOT_A_REDIS_FUNC', with args beginning with: 'THIS_IS_NOT_A_VALID_ARG" + ) + end end it 'records net.peer.name and net.peer.port attributes' do - expect do - Redis.new(host: 'example.com', port: 8321, timeout: 0.01).auth(password) - end.must_raise Redis::CannotConnectError - - _(last_span.name).must_equal 'AUTH' - _(last_span.attributes['db.system']).must_equal 'redis' - _(last_span.attributes['db.statement']).must_equal 'AUTH ?' - _(last_span.attributes['net.peer.name']).must_equal 'example.com' - _(last_span.attributes['net.peer.port']).must_equal 8321 + client = Redis.new(host: 'example.com', port: 8321, timeout: 0.01) + expect { client.auth(password) }.must_raise Redis::CannotConnectError + + if redis_gte_5? + skip( + 'Redis 5 is a wrapper around RedisClient, which calls' \ + '`ensure_connected` before any of the middlewares are invoked.' \ + 'This is more appropriately instrumented via a `#connect` hook in the middleware.' + ) + else + _(last_span.name).must_equal 'AUTH' + _(last_span.attributes['db.system']).must_equal 'redis' + _(last_span.attributes['db.statement']).must_equal 'AUTH ?' + _(last_span.attributes['net.peer.name']).must_equal 'example.com' + _(last_span.attributes['net.peer.port']).must_equal 8321 + end end it 'traces pipelined commands' do @@ -185,10 +224,11 @@ def redis_with_auth(redis_options = {}) it 'traces pipelined commands on commit' do redis = redis_with_auth - redis.queue([:set, 'v1', '0']) - redis.queue([:incr, 'v1']) - redis.queue([:get, 'v1']) - redis.commit + redis.pipelined do |pipeline| + pipeline.set('v1', '0') + pipeline.incr('v1') + pipeline.get('v1') + end _(exporter.finished_spans.size).must_equal 2 _(last_span.name).must_equal 'PIPELINED' @@ -225,16 +265,17 @@ def redis_with_auth(redis_options = {}) it 'truncates long db.statements' do redis = redis_with_auth the_long_value = 'y' * 100 - redis.queue([:set, 'v1', the_long_value]) - redis.queue([:set, 'v1', the_long_value]) - redis.queue([:set, 'v1', the_long_value]) - redis.queue([:set, 'v1', the_long_value]) - redis.queue([:set, 'v1', the_long_value]) - redis.queue([:set, 'v1', the_long_value]) - redis.queue([:set, 'v1', the_long_value]) - redis.queue([:set, 'v1', the_long_value]) - redis.queue([:set, 'v1', the_long_value]) - redis.commit + redis.pipelined do |pipeline| + pipeline.set('v1', the_long_value) + pipeline.set('v1', the_long_value) + pipeline.set('v1', the_long_value) + pipeline.set('v1', the_long_value) + pipeline.set('v1', the_long_value) + pipeline.set('v1', the_long_value) + pipeline.set('v1', the_long_value) + pipeline.set('v1', the_long_value) + pipeline.set('v1', the_long_value) + end expected_db_statement = <<~HEREDOC.chomp SET v1 yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy @@ -297,7 +338,7 @@ def redis_with_auth(redis_options = {}) _(exporter.finished_spans.size).must_equal 3 set_span = exporter.finished_spans[0] - _(set_span.name).must_equal 'AUTH' + _(set_span.name).must_equal(redis_gte_5? ? 'PIPELINED' : 'AUTH') _(set_span.attributes['db.system']).must_equal 'redis' _(set_span.attributes).wont_include('db.statement') @@ -326,7 +367,7 @@ def redis_with_auth(redis_options = {}) _(exporter.finished_spans.size).must_equal 3 set_span = exporter.finished_spans[0] - _(set_span.name).must_equal 'AUTH' + _(set_span.name).must_equal(redis_gte_5? ? 'PIPELINED' : 'AUTH') _(set_span.attributes['db.system']).must_equal 'redis' _(set_span.attributes['db.statement']).must_equal( 'AUTH ?' diff --git a/instrumentation/redis/test/opentelemetry/instrumentation/redis_client_test.rb b/instrumentation/redis/test/opentelemetry/instrumentation/redis_client_test.rb index 4b0e5bbe61..2ee40880bd 100644 --- a/instrumentation/redis/test/opentelemetry/instrumentation/redis_client_test.rb +++ b/instrumentation/redis/test/opentelemetry/instrumentation/redis_client_test.rb @@ -21,10 +21,12 @@ # will generate one extra span on connect because the Redis client will # send an AUTH command before doing anything else. def redis_with_auth(redis_options = {}) - redis_options[:password] = password - redis_options[:host] = redis_host - redis_options[:port] = redis_port - RedisClient.new(**redis_options) + redis_options[:password] ||= password + redis_options[:host] ||= redis_host + redis_options[:port] ||= redis_port + RedisClient.new(**redis_options).tap do |client| + client.send(:raw_connection) # force lazy client to connect + end end before do @@ -45,7 +47,7 @@ def redis_with_auth(redis_options = {}) it 'accepts peer service name from config' do instrumentation.instance_variable_set(:@installed, false) instrumentation.install(peer_service: 'readonly:redis') - Redis.new(host: redis_host, port: redis_port).auth(password) + redis_with_auth _(last_span.attributes['peer.service']).must_equal 'readonly:redis' end @@ -63,7 +65,31 @@ def redis_with_auth(redis_options = {}) end it 'after authorization with Redis server' do - Redis.new(host: redis_host, port: redis_port).auth(password) + client = redis_with_auth + + _(client.connected?).must_equal(true) + + _(last_span.name).must_equal 'PIPELINED' + _(last_span.attributes['db.system']).must_equal 'redis' + _(last_span.attributes['db.statement']).must_equal 'HELLO ? ? ? ?' + _(last_span.attributes['net.peer.name']).must_equal redis_host + _(last_span.attributes['net.peer.port']).must_equal redis_port + end + + it 'after calling auth lowercase' do + client = redis_with_auth + client.call('auth', password) + + _(last_span.name).must_equal 'AUTH' + _(last_span.attributes['db.system']).must_equal 'redis' + _(last_span.attributes['db.statement']).must_equal 'AUTH ?' + _(last_span.attributes['net.peer.name']).must_equal redis_host + _(last_span.attributes['net.peer.port']).must_equal redis_port + end + + it 'after calling AUTH uppercase' do + client = redis_with_auth + client.call('AUTH', password) _(last_span.name).must_equal 'AUTH' _(last_span.attributes['db.system']).must_equal 'redis' @@ -157,9 +183,10 @@ def redis_with_auth(redis_options = {}) it 'records net.peer.name and net.peer.port attributes' do expect do - Redis.new(host: 'example.com', port: 8321, timeout: 0.01).auth(password) - end.must_raise Redis::CannotConnectError + redis_with_auth(host: 'example.com', port: 8321, timeout: 0.01) + end.must_raise RedisClient::CannotConnectError + skip('FIXME: what is this intended to test?') _(last_span.name).must_equal 'AUTH' _(last_span.attributes['db.system']).must_equal 'redis' _(last_span.attributes['db.statement']).must_equal 'AUTH ?'