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..dc2e44fe5f 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 } @@ -95,6 +109,8 @@ def redis_with_auth(redis_options = {}) end it 'reflects db index' do + skip if redis_gte_5? + redis = redis_with_auth(db: 1) redis.get('K') @@ -102,12 +118,40 @@ def redis_with_auth(redis_options = {}) 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') + + get_span = exporter.finished_spans.last + + _(select_span.attributes['db.system']).must_equal 'redis' _(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.name).must_equal 'GET' + _(get_span.attributes['db.system']).must_equal 'redis' + _(get_span.attributes['db.statement']).must_equal('GET K') + _(get_span.attributes['db.redis.database_index']).must_equal 1 + _(get_span.attributes['net.peer.name']).must_equal redis_host + _(get_span.attributes['net.peer.port']).must_equal redis_port + end + + it 'reflects db index v5' do + skip unless redis_gte_5? + + redis = redis_with_auth(db: 1) + redis.get('K') + + _(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") + + _(select_span.attributes['db.system']).must_equal 'redis' + _(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.name).must_equal 'GET' _(get_span.attributes['db.system']).must_equal 'redis' _(get_span.attributes['db.statement']).must_equal('GET K') @@ -134,6 +178,8 @@ def redis_with_auth(redis_options = {}) end it 'records exceptions' do + skip if redis_gte_5? + expect do redis = redis_with_auth redis.call 'THIS_IS_NOT_A_REDIS_FUNC', 'THIS_IS_NOT_A_VALID_ARG' @@ -150,21 +196,68 @@ 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" ) end - it 'records net.peer.name and net.peer.port attributes' do + it 'records exceptions v5' do + skip unless redis_gte_5? + expect do - Redis.new(host: 'example.com', port: 8321, timeout: 0.01).auth(password) - end.must_raise Redis::CannotConnectError + redis = redis_with_auth + redis.call 'THIS_IS_NOT_A_REDIS_FUNC', 'THIS_IS_NOT_A_VALID_ARG' + end.must_raise Redis::CommandError - _(last_span.name).must_equal 'AUTH' + _(exporter.finished_spans.size).must_equal 2 + _(last_span.name).must_equal 'THIS_IS_NOT_A_REDIS_FUNC' _(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 + _(last_span.attributes['db.statement']).must_equal( + 'THIS_IS_NOT_A_REDIS_FUNC THIS_IS_NOT_A_VALID_ARG' + ) + _(last_span.attributes['net.peer.name']).must_equal redis_host + _(last_span.attributes['net.peer.port']).must_equal redis_port + _(last_span.status.code).must_equal( + OpenTelemetry::Trace::Status::ERROR + ) + + _(last_span.status.description.tr('`', "'")).must_include( + 'Unhandled exception of type: RedisClient::CommandError' + ) + end + + it 'records net.peer.name and net.peer.port attributes' do + skip if redis_gte_5? + + 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 'records net.peer.name and net.peer.port attributes v5' do + skip unless redis_gte_5? + + client = Redis.new(host: 'example.com', port: 8321, timeout: 0.01) + expect { client.auth(password) }.must_raise Redis::CannotConnectError + + # NOTE: Redis 5 is a wrapper around RedisClient, which calls + # ensure_connected` before any of the middlewares are invoked, so we don't + # have a span here. This is more appropriately instrumented via a `#connect` + # hook in the middleware. end it 'traces pipelined commands' do @@ -185,10 +278,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 +319,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 @@ -291,13 +386,39 @@ def redis_with_auth(redis_options = {}) end it 'omits db.statement attribute' do + skip if redis_gte_5? + + redis = redis_with_auth + _(redis.set('K', 'xyz')).must_equal 'OK' + _(redis.get('K')).must_equal 'xyz' + _(exporter.finished_spans.size).must_equal 3 + + set_span = exporter.finished_spans[0] + _(set_span.name).must_equal('AUTH') + _(set_span.attributes['db.system']).must_equal 'redis' + _(set_span.attributes).wont_include('db.statement') + + set_span = exporter.finished_spans[1] + _(set_span.name).must_equal 'SET' + _(set_span.attributes['db.system']).must_equal 'redis' + _(set_span.attributes).wont_include('db.statement') + + set_span = exporter.finished_spans[2] + _(set_span.name).must_equal 'GET' + _(set_span.attributes['db.system']).must_equal 'redis' + _(set_span.attributes).wont_include('db.statement') + end + + it 'omits db.statement attribute v5' do + skip unless redis_gte_5? + redis = redis_with_auth _(redis.set('K', 'xyz')).must_equal 'OK' _(redis.get('K')).must_equal 'xyz' _(exporter.finished_spans.size).must_equal 3 set_span = exporter.finished_spans[0] - _(set_span.name).must_equal 'AUTH' + _(set_span.name).must_equal('PIPELINED') _(set_span.attributes['db.system']).must_equal 'redis' _(set_span.attributes).wont_include('db.statement') @@ -320,13 +441,45 @@ def redis_with_auth(redis_options = {}) end it 'obfuscates arguments in db.statement' do + skip if redis_gte_5? + + redis = redis_with_auth + _(redis.set('K', 'xyz')).must_equal 'OK' + _(redis.get('K')).must_equal 'xyz' + _(exporter.finished_spans.size).must_equal 3 + + set_span = exporter.finished_spans[0] + _(set_span.name).must_equal('AUTH') + _(set_span.attributes['db.system']).must_equal 'redis' + _(set_span.attributes['db.statement']).must_equal( + 'AUTH ?' + ) + + set_span = exporter.finished_spans[1] + _(set_span.name).must_equal 'SET' + _(set_span.attributes['db.system']).must_equal 'redis' + _(set_span.attributes['db.statement']).must_equal( + 'SET ? ?' + ) + + set_span = exporter.finished_spans[2] + _(set_span.name).must_equal 'GET' + _(set_span.attributes['db.system']).must_equal 'redis' + _(set_span.attributes['db.statement']).must_equal( + 'GET ?' + ) + end + + it 'obfuscates arguments in db.statement v5' do + skip unless redis_gte_5? + redis = redis_with_auth _(redis.set('K', 'xyz')).must_equal 'OK' _(redis.get('K')).must_equal 'xyz' _(exporter.finished_spans.size).must_equal 3 set_span = exporter.finished_spans[0] - _(set_span.name).must_equal 'AUTH' + _(set_span.name).must_equal('PIPELINED') _(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..b65a451cbe 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' @@ -155,16 +181,18 @@ def redis_with_auth(redis_options = {}) ) end - it 'records net.peer.name and net.peer.port attributes' do + it 'connect is uninstrumented' 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 + redis_with_auth(host: 'example.com', port: 8321, timeout: 0.01) + end.must_raise RedisClient::CannotConnectError + + # NOTE: RedisClient runs `ensure_connected` before Otel's instrumentation + # span is created. The 'connect' operation can be separately instrumented + # via the connect hook in the middleware. If this expectation (last_span must be nil) + # fails due to this implementation, it can be removed. + # This test remains here for parity the the V4 instrumentation, which _does_ + # wrap the connect failure in a span. + _(last_span).must_be_nil end it 'traces pipelined commands' do