add metrics tests

Author: zvkemp

instrumentation/redis/Gemfile

@@ -12,4 +12,6 @@ group :test do
   gem 'opentelemetry-instrumentation-base', path: '../base'
 end
 
+gem 'opentelemetry-test-helpers', github: 'zvkemp/opentelemetry-ruby', glob: 'test_helpers/*.gemspec', require: false, ref: 'test-helpers-metrics'
+
 gem 'pry-byebug'

instrumentation/redis/lib/opentelemetry/instrumentation/redis/instrumentation.rb

@@ -26,9 +26,9 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base
 
         # https://opentelemetry.io/docs/specs/semconv/database/database-metrics/#metric-dbclientoperationduration
         histogram 'db.client.operation.duration',
-                  attributes: { 'db.system.name' => 'redis' },
+                  attributes: { 'db.system' => 'redis' },
                   unit: 's',
-                  boundaries: [0.001, 0.005, 0.01, 0.05, 0.1, 0.5, 1, 5, 10]
+                  explicit_bucket_boundaries: [0.001, 0.005, 0.01, 0.05, 0.1, 0.5, 1, 5, 10]
 
         def client_operation_duration_histogram
           histogram('db.client.operation.duration')
@@ -37,18 +37,42 @@ def client_operation_duration_histogram
         private
 
         def require_dependencies
-          require_relative 'patches/redis_v4_client' if defined?(::Redis) && ::Redis::VERSION < '5'
+          require_redis_client_dependencies
+          require_redis_v4_dependencies
+        end
+
+        def require_redis_v4_dependencies
+          return unless defined?(::Redis) && Gem::Version.new(Redis::VERSION) < Gem::Version.new('5.0.0')
+
+          require_relative 'patches/redis_v4_client'
+          require_relative 'patches/redis_v4_client_metrics'
+        end
 
+        def require_redis_client_dependencies
           return unless defined?(::RedisClient)
 
           require_relative 'middlewares/redis_client'
           require_relative 'middlewares/redis_client_metrics'
         end
 
         def patch_client
-          ::RedisClient.register(Middlewares::RedisClientInstrumentation) if defined?(::RedisClient)
-          ::RedisClient.register(Middlewares::RedisClientMetrics) if defined?(::RedisClient)
-          ::Redis::Client.prepend(Patches::RedisV4Client) if defined?(::Redis) && ::Redis::VERSION < '5'
+          patch_redis_v4_client
+          patch_redis_client
+        end
+
+        def patch_redis_v4_client
+          return unless defined?(::Redis) && Gem::Version.new(Redis::VERSION) < Gem::Version.new('5.0.0')
+
+          ::Redis::Client.prepend(Patches::RedisV4Client)
+          ::Redis::Client.prepend(Patches::RedisV4ClientMetrics) if metrics_defined?
+        end
+
+        # Applies to redis-client or redis >= 5
+        def patch_redis_client
+          return unless defined?(::RedisClient)
+
+          ::RedisClient.register(Middlewares::RedisClientInstrumentation)
+          ::RedisClient.register(Middlewares::RedisClientMetrics) if metrics_defined?
         end
       end
     end

instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client_metrics.rb

@@ -4,55 +4,41 @@
 #
 # SPDX-License-Identifier: Apache-2.0
 
+require_relative '../patches/metrics_helpers'
+
 module OpenTelemetry
   module Instrumentation
     module Redis
       module Middlewares
         # Adapter for redis-client instrumentation interface
         module RedisClientMetrics
+          def self.included(base)
+            base.include(OpenTelemetry::Instrumentation::Redis::Patches::MetricsHelpers)
+          end
+
           def call(command, redis_config)
             return super unless (histogram = instrumentation.client_operation_duration_histogram)
 
-            timed(histogram, command.first, redis_config) do
+            attributes = metric_attributes(redis_config, command.first)
+            otel_record_histogram(histogram, attributes) do
               super
             end
           end
 
           def call_pipelined(commands, redis_config)
             return super unless (histogram = instrumentation.client_operation_duration_histogram)
 
-            timed(histogram, 'PIPELINE', redis_config) do
+            attributes = metric_attributes(redis_config, 'PIPELINED')
+            otel_record_histogram(histogram, attributes) do
               super
             end
           end
 
           private
 
-          def timed(histogram, operation_name, redis_config)
-            t0 = monotonic_now
-
-            yield.tap do
-              duration = monotonic_now - t0
-
-              histogram.record(duration, attributes: metric_attributes(redis_config, operation_name))
-            end
-          end
-
-          def monotonic_now
-            Process.clock_gettime(Process::CLOCK_MONOTONIC, :float_second)
-          end
-
           def metric_attributes(redis_config, operation_name)
-            attributes = {
-              'db.system' => 'redis',
-              'db.operation.name' => operation_name,
-              'net.peer.name' => redis_config.host,
-              'net.peer.port' => redis_config.port
-            }
-
-            attributes['db.redis.database_index'] = redis_config.db unless redis_config.db.zero?
-            attributes['peer.service'] = instrumentation.config[:peer_service] if instrumentation.config[:peer_service]
-            attributes.merge!(OpenTelemetry::Instrumentation::Redis.attributes)
+            attributes = span_attributes(redis_config)
+            attributes['db.operation.name'] = operation_name
             attributes
           end
 

instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/redis_v4_client.rb

@@ -4,6 +4,8 @@
 #
 # SPDX-License-Identifier: Apache-2.0
 
+require_relative 'metrics_helpers'
+
 module OpenTelemetry
   module Instrumentation
     module Redis
@@ -16,18 +18,7 @@ module RedisV4Client
           def process(commands)
             return super unless instrumentation_config[:trace_root_spans] || OpenTelemetry::Trace.current_span.context.valid?
 
-            host = options[:host]
-            port = options[:port]
-
-            attributes = {
-              'db.system' => 'redis',
-              'net.peer.name' => host,
-              'net.peer.port' => port
-            }
-
-            attributes['db.redis.database_index'] = options[:db] unless options[:db].zero?
-            attributes['peer.service'] = instrumentation_config[:peer_service] if instrumentation_config[:peer_service]
-            attributes.merge!(OpenTelemetry::Instrumentation::Redis.attributes)
+            attributes = otel_base_attributes
 
             unless instrumentation_config[:db_statement] == :omit
               parsed_commands = parse_commands(commands)
@@ -88,6 +79,26 @@ def instrumentation_tracer
           def instrumentation_config
             Redis::Instrumentation.instance.config
           end
+
+          def instrumentation
+            Redis::Instrumentation.instance
+          end
+
+          def otel_base_attributes
+            host = options[:host]
+            port = options[:port]
+
+            attributes = {
+              'db.system' => 'redis',
+              'net.peer.name' => host,
+              'net.peer.port' => port
+            }
+
+            attributes['db.redis.database_index'] = options[:db] unless options[:db].zero?
+            attributes['peer.service'] = instrumentation_config[:peer_service] if instrumentation_config[:peer_service]
+            attributes.merge!(OpenTelemetry::Instrumentation::Redis.attributes)
+            attributes
+          end
         end
       end
     end

instrumentation/redis/opentelemetry-instrumentation-redis.gemspec

@@ -32,7 +32,7 @@ Gem::Specification.new do |spec|
   spec.add_development_dependency 'bundler', '~> 2.4'
   spec.add_development_dependency 'minitest', '~> 5.0'
   spec.add_development_dependency 'opentelemetry-sdk', '~> 1.1'
-  spec.add_development_dependency 'opentelemetry-test-helpers', '~> 0.3'
+  spec.add_development_dependency 'opentelemetry-test-helpers', '~> 0.5'
   spec.add_development_dependency 'rubocop', '~> 1.71.0'
   spec.add_development_dependency 'rubocop-performance', '~> 1.23.0'
   spec.add_development_dependency 'simplecov', '~> 0.17.1'

instrumentation/redis/test/opentelemetry/instrumentation/redis/patches/client_test.rb

@@ -41,9 +41,10 @@ def redis_gte_5?
     redis_version_major&.>=(5)
   end
 
+  let(:config) { { db_statement: :include } }
+
   before do
     # ensure obfuscation is off if it was previously set in a different test
-    config = { db_statement: :include }
     instrumentation.install(config)
     exporter.reset
   end
@@ -389,4 +390,96 @@ def redis_gte_5?
       end
     end
   end
+
+  if defined?(OpenTelemetry::Metrics)
+    describe 'metrics not enabled' do
+      it 'will not be enabled' do
+        assert(instrumentation.metrics_defined?)
+        refute(instrumentation.metrics_enabled?)
+      end
+    end
+
+    describe 'metrics enabled' do
+      let(:config) { { db_statement: :include, metrics: true } }
+      let(:metric_snapshot) do
+        metrics_exporter.pull
+        metrics_exporter.metric_snapshots.last
+      end
+
+      it 'will be enabled' do
+        assert(instrumentation.metrics_defined?)
+        assert(instrumentation.metrics_enabled?)
+      end
+
+      it 'works', with_metrics_sdk: true do
+        skip if redis_gte_5?
+
+        redis = redis_with_auth
+        key = SecureRandom.hex
+        10.times { redis.incr(key) }
+        redis.expire(key, 1)
+
+        _(metric_snapshot.data_points.length).must_equal(3)
+
+        metric_snapshot.data_points.each do |data_point|
+          _(data_point.attributes['db.system']).must_equal('redis')
+        end
+
+        by_operation_name = metric_snapshot.data_points.each_with_object({}) { |d, res| res[d.attributes['db.operation.name']] = d }
+        _(by_operation_name.keys.sort).must_equal(%w[auth expire incr])
+
+        _(by_operation_name['auth'].count).must_equal(1)
+        _(by_operation_name['incr'].count).must_equal(10)
+        _(by_operation_name['expire'].count).must_equal(1)
+      end
+
+      it 'works v5', with_metrics_sdk: true do
+        skip unless redis_gte_5?
+
+        redis = redis_with_auth
+        key = SecureRandom.hex
+        10.times { redis.incr(key) }
+        redis.expire(key, 1)
+
+        _(metric_snapshot.data_points.length).must_equal(3)
+
+        metric_snapshot.data_points.each do |data_point|
+          _(data_point.attributes['db.system']).must_equal('redis')
+        end
+
+        by_operation_name = metric_snapshot.data_points.each_with_object({}) { |d, res| res[d.attributes['db.operation.name']] = d }
+        _(by_operation_name.keys.sort).must_equal(%w[PIPELINED expire incr])
+
+        _(by_operation_name['PIPELINED'].count).must_equal(1)
+        _(by_operation_name['incr'].count).must_equal(10)
+        _(by_operation_name['expire'].count).must_equal(1)
+      end
+
+      it 'adds errors', with_metrics_sdk: true do
+        skip if redis_gte_5?
+
+        redis = redis_with_auth
+        key = SecureRandom.hex
+        redis.setex(key, 100, 'string_value')
+        expect { redis.incr(key) }.must_raise(Redis::CommandError)
+
+        last_data_point = metric_snapshot.data_points.last
+        _(last_data_point.attributes['db.operation.name']).must_equal('incr')
+        _(last_data_point.attributes['error.type']).must_equal('RedisClient::CommandError')
+      end
+
+      it 'adds errors v5', with_metrics_sdk: true do
+        skip unless redis_gte_5?
+
+        redis = redis_with_auth
+        key = SecureRandom.hex
+        redis.setex(key, 100, 'string_value')
+        expect { redis.incr(key) }.must_raise(Redis::CommandError)
+
+        last_data_point = metric_snapshot.data_points.last
+        _(last_data_point.attributes['db.operation.name']).must_equal('incr')
+        _(last_data_point.attributes['error.type']).must_equal('RedisClient::CommandError')
+      end
+    end
+  end
 end unless ENV['OMIT_SERVICES']

instrumentation/redis/test/test_helper.rb

@@ -21,3 +21,4 @@
   c.logger = Logger.new($stderr, level: ENV.fetch('OTEL_LOG_LEVEL', 'fatal').to_sym)
   c.add_span_processor span_processor
 end
+require 'opentelemetry/test_helpers/metrics'