Merge branch 'main' into ruby-3.4

Author: arielvalentin

instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers/action_controller.rb

@@ -53,7 +53,7 @@ def finish(_name, _id, payload)
           # @return [Array<String, Hash>] the span name and attributes
           def to_span_name_and_attributes(payload)
             request = payload[:request]
-            http_route = request.route_uri_pattern if request.respond_to?(:route_uri_pattern)
+            http_route = request.route_uri_pattern.chomp('(.:format)') if request.respond_to?(:route_uri_pattern)
 
             attributes = {
               OpenTelemetry::SemanticConventions::Trace::CODE_NAMESPACE => String(payload[:controller]),
@@ -63,7 +63,7 @@ def to_span_name_and_attributes(payload)
             attributes[OpenTelemetry::SemanticConventions::Trace::HTTP_TARGET] = request.filtered_path if request.filtered_path != request.fullpath
 
             if @span_naming == :semconv
-              return ["#{request.method} #{http_route.gsub('(.:format)', '')}", attributes] if http_route
+              return ["#{request.method} #{http_route}", attributes] if http_route
 
               return ["#{request.method} /#{payload.dig(:params, :controller)}/#{payload.dig(:params, :action)}", attributes]
             end

instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/handlers/action_controller_test.rb

@@ -58,6 +58,14 @@
     _(span.attributes['code.function']).must_equal 'ok'
   end
 
+  it 'strips (:format) from http.route' do
+    skip "Rails #{Rails.gem_version} does not define ActionDispatch::Request#route_uri_pattern" if Rails.gem_version < Gem::Version.new('7.1')
+
+    get 'items/1234'
+
+    _(span.attributes['http.route']).must_equal '/items/:id'
+  end
+
   it 'does not memoize data across requests' do
     get '/ok'
     get '/items/new'

instrumentation/aws_lambda/test/opentelemetry/instrumentation_test.rb

@@ -76,7 +76,8 @@
         _(last_span.hex_span_id.size).must_equal 16
         _(last_span.hex_trace_id.size).must_equal 32
         _(last_span.trace_flags.sampled?).must_equal true
-        _(last_span.tracestate.to_h.to_s).must_equal '{}'
+
+        assert_equal last_span.tracestate, {}
       end
     end
 
@@ -95,7 +96,7 @@
         _(last_span.hex_span_id.size).must_equal 16
         _(last_span.hex_trace_id.size).must_equal 32
         _(last_span.trace_flags.sampled?).must_equal true
-        _(last_span.tracestate.to_h).must_equal { 'otel' => 'ff40ea9699e62af2-01' }
+        _(last_span.tracestate).must_equal({ 'otel' => 'ff40ea9699e62af2-01' })
       end
       event_v1['headers'].delete('traceparent')
       event_v1['headers'].delete('tracestate')
@@ -195,7 +196,8 @@
         _(last_span.hex_span_id.size).must_equal 16
         _(last_span.hex_trace_id.size).must_equal 32
         _(last_span.trace_flags.sampled?).must_equal true
-        _(last_span.tracestate.to_h.to_s).must_equal '{}'
+
+        assert_equal last_span.tracestate, {}
       end
     end
 

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

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

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,19 +109,49 @@ 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')
 
       _(exporter.finished_spans.size).must_equal 3
 
       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 ?'