From c3127c8fb978426772a3a00ee25b57c67aa5494a Mon Sep 17 00:00:00 2001
From: zvkemp <zvkemp@gmail.com>
Date: Thu, 30 Jan 2025 13:58:16 -0500
Subject: [PATCH] add metrics tests
---
instrumentation/redis/Gemfile | 1 +
.../instrumentation/redis/instrumentation.rb | 36 +++++--
.../redis/middlewares/redis_client_metrics.rb | 22 +++--
.../redis/patches/client_test.rb | 95 ++++++++++++++++++-
instrumentation/redis/test/test_helper.rb | 2 +
5 files changed, 139 insertions(+), 17 deletions(-)
diff --git a/instrumentation/redis/Gemfile b/instrumentation/redis/Gemfile
index 8cc6b11452..9cfd4ca992 100644
--- a/instrumentation/redis/Gemfile
+++ b/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'
diff --git a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/instrumentation.rb b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/instrumentation.rb
index d201fd7355..328fc353fc 100644
--- a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/instrumentation.rb
+++ b/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,8 +37,18 @@ 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'
@@ -46,9 +56,23 @@ def require_dependencies
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
diff --git a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client_metrics.rb b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client_metrics.rb
index 1f82095913..8ecfd7a027 100644
--- a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client_metrics.rb
+++ b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client_metrics.rb
@@ -13,7 +13,8 @@ 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
@@ -21,21 +22,23 @@ def call(command, redis_config)
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
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 7fbf262c27..4a50b08f82 100644
--- a/instrumentation/redis/test/opentelemetry/instrumentation/redis/patches/client_test.rb
+++ b/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']
diff --git a/instrumentation/redis/test/test_helper.rb b/instrumentation/redis/test/test_helper.rb
index 9375d9e5e6..1835b257e9 100644
--- a/instrumentation/redis/test/test_helper.rb
+++ b/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'