diff --git a/instrumentation/redis/Gemfile b/instrumentation/redis/Gemfile index 8cc6b1145..bcf6e8057 100644 --- a/instrumentation/redis/Gemfile +++ b/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' diff --git a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/instrumentation.rb b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/instrumentation.rb index d201fd735..328fc353f 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 1f8209591..83922d20a 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 @@ -4,16 +4,23 @@ # # 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 @@ -21,38 +28,17 @@ 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_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 diff --git a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/metrics_helpers.rb b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/metrics_helpers.rb new file mode 100644 index 000000000..8108e1739 --- /dev/null +++ b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/metrics_helpers.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +# Copyright The OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +module OpenTelemetry + module Instrumentation + module Redis + module Patches + # Common logic for tracking histograms and other metrics instruments + module MetricsHelpers + private + + def otel_record_histogram(histogram, attributes) + t0 = otel_monotonic_now + yield.tap do |result| + attributes['error.type'] = result.class.to_s if result.is_a?(StandardError) + end + rescue StandardError => e + attributes['error.type'] = e.class.to_s + raise + ensure + duration = otel_monotonic_now - t0 + histogram.record(duration, attributes: attributes) + end + + def otel_monotonic_now + Process.clock_gettime(Process::CLOCK_MONOTONIC, :float_second) + end + end + end + end + end +end diff --git a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/redis_v4_client.rb b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/redis_v4_client.rb index 6bd473c09..27aac5508 100644 --- a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/redis_v4_client.rb +++ b/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 diff --git a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/redis_v4_client_metrics.rb b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/redis_v4_client_metrics.rb new file mode 100644 index 000000000..f13d1a63b --- /dev/null +++ b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/redis_v4_client_metrics.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +# Copyright The OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +module OpenTelemetry + module Instrumentation + module Redis + module Patches + # Module to prepend to Redis::Client for metrics + module RedisV4ClientMetrics + def self.prepended(base) + base.prepend(OpenTelemetry::Instrumentation::Redis::Patches::MetricsHelpers) + end + + def process(commands) + return super unless (histogram = instrumentation.client_operation_duration_histogram) + + attributes = otel_base_attributes + + attributes['db.operation.name'] = + if commands.length == 1 + commands[0][0].to_s + else + 'PIPELINED' + end + + otel_record_histogram(histogram, attributes) { super } + end + end + end + end + end +end diff --git a/instrumentation/redis/opentelemetry-instrumentation-redis.gemspec b/instrumentation/redis/opentelemetry-instrumentation-redis.gemspec index 2b87e0966..76e21f9f3 100644 --- a/instrumentation/redis/opentelemetry-instrumentation-redis.gemspec +++ b/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' 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 7fbf262c2..07ef83a61 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,97 @@ 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_be_instance_of(String) + _(last_data_point.attributes['error.type']).must_equal('Redis::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 9375d9e5e..e95cf9e1a 100644 --- a/instrumentation/redis/test/test_helper.rb +++ b/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'