From a2c333c336461b3431a5494bab2c78df92f76c4a Mon Sep 17 00:00:00 2001 From: Tony Hsu Date: Wed, 6 Mar 2024 00:33:33 +0100 Subject: [PATCH 1/2] feat: Skip `ping` span with option `ping_enabled` --- .../trilogy/instrumentation.rb | 1 + .../instrumentation/trilogy/patches/client.rb | 2 ++ .../trilogy/instrumentation_test.rb | 34 ++++++++++++++----- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/instrumentation.rb b/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/instrumentation.rb index 684fbe660..ea353cb49 100644 --- a/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/instrumentation.rb +++ b/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/instrumentation.rb @@ -28,6 +28,7 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base option :span_name, default: :statement_type, validate: %I[statement_type db_name db_operation_and_name] option :obfuscation_limit, default: 2000, validate: :integer option :propagator, default: nil, validate: :string + option :ping_enabled, default: true, validate: :boolean attr_reader :propagator diff --git a/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb b/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb index d5e93e7a8..5c2e69189 100644 --- a/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb +++ b/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb @@ -26,6 +26,8 @@ def initialize(options = {}) end def ping(...) + return super unless config[:ping_enabled] + tracer.in_span( 'ping', attributes: client_attributes.merge!(OpenTelemetry::Instrumentation::Trilogy.attributes), diff --git a/instrumentation/trilogy/test/opentelemetry/instrumentation/trilogy/instrumentation_test.rb b/instrumentation/trilogy/test/opentelemetry/instrumentation/trilogy/instrumentation_test.rb index 9b1b7b845..82ea8fd13 100644 --- a/instrumentation/trilogy/test/opentelemetry/instrumentation/trilogy/instrumentation_test.rb +++ b/instrumentation/trilogy/test/opentelemetry/instrumentation/trilogy/instrumentation_test.rb @@ -182,18 +182,34 @@ end describe 'when pinging' do - let(:span) { exporter.finished_spans[2] } + describe 'when `ping_enabled` is `true`' do + let(:config) { { ping_enabled: true } } + let(:span) { exporter.finished_spans[2] } - it 'spans will include database name' do - _(client.connected_host).wont_be_nil + it 'spans will include database name' do + _(client.connected_host).wont_be_nil - client.ping + client.ping - _(span.name).must_equal 'ping' - _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_NAME]).must_equal(database) - _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_USER]).must_equal(username) - _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_SYSTEM]).must_equal 'mysql' - _(span.attributes[OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME]).must_equal(host) + _(span.name).must_equal 'ping' + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_NAME]).must_equal(database) + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_USER]).must_equal(username) + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_SYSTEM]).must_equal 'mysql' + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME]).must_equal(host) + end + end + + describe 'when `ping_enabled` is `false`' do + let(:config) { { ping_enabled: false } } + let(:span) { exporter.finished_spans[2] } + + it 'does not generate `ping` span' do + _(client.connected_host).wont_be_nil + + client.ping + + _(span).must_be_nil + end end end From 8049f2c3752ad90b0053ef3cd6b72c77deb5bdd5 Mon Sep 17 00:00:00 2001 From: Tony Hsu Date: Thu, 7 Mar 2024 00:31:27 +0100 Subject: [PATCH 2/2] feat: Set `ping_enabled` option default to `false` and enable with parent --- .../trilogy/instrumentation.rb | 2 +- .../instrumentation/trilogy/patches/client.rb | 2 +- .../trilogy/instrumentation_test.rb | 46 ++++++++++++++++++- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/instrumentation.rb b/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/instrumentation.rb index ea353cb49..f78c45fd6 100644 --- a/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/instrumentation.rb +++ b/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/instrumentation.rb @@ -28,7 +28,7 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base option :span_name, default: :statement_type, validate: %I[statement_type db_name db_operation_and_name] option :obfuscation_limit, default: 2000, validate: :integer option :propagator, default: nil, validate: :string - option :ping_enabled, default: true, validate: :boolean + option :ping_enabled, default: false, validate: :boolean attr_reader :propagator diff --git a/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb b/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb index 5c2e69189..e760be23e 100644 --- a/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb +++ b/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb @@ -26,7 +26,7 @@ def initialize(options = {}) end def ping(...) - return super unless config[:ping_enabled] + return super unless OpenTelemetry::Trace.current_span.context.valid? || config[:ping_enabled] tracer.in_span( 'ping', diff --git a/instrumentation/trilogy/test/opentelemetry/instrumentation/trilogy/instrumentation_test.rb b/instrumentation/trilogy/test/opentelemetry/instrumentation/trilogy/instrumentation_test.rb index 82ea8fd13..3deefd6f0 100644 --- a/instrumentation/trilogy/test/opentelemetry/instrumentation/trilogy/instrumentation_test.rb +++ b/instrumentation/trilogy/test/opentelemetry/instrumentation/trilogy/instrumentation_test.rb @@ -182,9 +182,36 @@ end describe 'when pinging' do + let(:span) { exporter.finished_spans[2] } + + describe 'when default' do + it 'does not generate `ping` span' do + _(client.connected_host).wont_be_nil + + client.ping + + _(span).must_be_nil + end + + describe 'but with parent' do + it 'spans will include database name' do + _(client.connected_host).wont_be_nil + + OpenTelemetry.tracer_provider.tracer.in_span('parent') do + client.ping + end + + _(span.name).must_equal 'ping' + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_NAME]).must_equal(database) + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_USER]).must_equal(username) + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_SYSTEM]).must_equal 'mysql' + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME]).must_equal(host) + end + end + end + describe 'when `ping_enabled` is `true`' do let(:config) { { ping_enabled: true } } - let(:span) { exporter.finished_spans[2] } it 'spans will include database name' do _(client.connected_host).wont_be_nil @@ -201,7 +228,6 @@ describe 'when `ping_enabled` is `false`' do let(:config) { { ping_enabled: false } } - let(:span) { exporter.finished_spans[2] } it 'does not generate `ping` span' do _(client.connected_host).wont_be_nil @@ -210,6 +236,22 @@ _(span).must_be_nil end + + describe 'but with parent' do + it 'spans will include database name' do + _(client.connected_host).wont_be_nil + + OpenTelemetry.tracer_provider.tracer.in_span('parent') do + client.ping + end + + _(span.name).must_equal 'ping' + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_NAME]).must_equal(database) + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_USER]).must_equal(username) + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::DB_SYSTEM]).must_equal 'mysql' + _(span.attributes[OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME]).must_equal(host) + end + end end end