add metrics tests

Author: zvkemp

instrumentation/redis/Gemfile

@@ -10,6 +10,7 @@ gemspec
 
 group :test do
   gem 'opentelemetry-instrumentation-base', path: '../base'
+  gem 'opentelemetry-metrics-test-helpers', path: '../../helpers/metrics-test-helpers', require: false
 end
 
 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

@@ -13,29 +13,32 @@ module RedisClientMetrics
           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_timed(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_timed(histogram, attributes) do
               super
             end
           end
 
           private
 
-          def timed(histogram, operation_name, redis_config)
+          def otel_timed(histogram, attributes)
             t0 = monotonic_now
-
-            yield.tap do
-              duration = monotonic_now - t0
-
-              histogram.record(duration, attributes: metric_attributes(redis_config, operation_name))
-            end
+            yield
+          rescue StandardError => e
+            attributes['error.type'] = e.class.to_s
+            raise
+          ensure
+            duration = monotonic_now - t0
+            histogram.record(duration, attributes: attributes)
           end
 
           def monotonic_now
@@ -44,7 +47,6 @@ def monotonic_now
 
           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

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,5 @@
   c.logger = Logger.new($stderr, level: ENV.fetch('OTEL_LOG_LEVEL', 'fatal').to_sym)
   c.add_span_processor span_processor
 end
+
+require 'opentelemetry-metrics-test-helpers'