Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Skip Trilogyping span with option ping_enabled #899

Closed
wants to merge 3 commits into from
Closed

feat: Skip Trilogyping span with option ping_enabled #899

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a2c333c
feat: Skip `ping` span with option `ping_enabled`
TonyCTHsu Mar 5, 2024
8049f2c
feat: Set `ping_enabled` option default to `false` and enable with
TonyCTHsu Mar 6, 2024
9da4814
Merge branch 'main' into feat/trilogy-skip-ping
kaylareopelle Apr 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/instrumentation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I just have two questions based on the original issue

IMO we should default to not tracing ping if it doesn't have a parent and allow a config option to opt in.

  1. Should this option be false as default?
  2. I'm not familiar with mysql2 ping operation, so I'm not sure in what case the ping operation will have a parent span. Potentially, a scenario involving a rack span as the parent, with other spans acting as child spans if it's a Rails or Sinatra application invoking ActiveRecord. Do you have any recommendations for addressing cases where a parent span exists and enabling the ping accordingly?

Copy link
Author

@TonyCTHsu TonyCTHsu Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed with: 8049f2c

  1. Should this option be false as default?

I set it as true originally because that would result in no data lost and no configuration changes for existing user. I don't mind the option being false. However, I would like to learn whether such change should be considered as a breaking change (major in SemVer2) or behavior change (minor in SemVer2)?

so I'm not sure in what case the ping operation will have a parent span.

I can imagine someone is building a /healthcheck endpoint that implements pinging databases. However, I would feel strange to see the ping span generated if I configured ping_enabled as false (Which also means that where is no way to silence it entirely.)

I am not familiar with OTel's public interfaces but I tried to simulate such scenario in test with

OpenTelemetry.tracer_provider.tracer.in_span('parent') do
  client.ping
end

Let me know if I am missing something or wrong about this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set it as true originally because that would result in no data lost and no configuration changes for existing user.

Make sense. Maybe ask it in next SIG meeting (also breaking vs behavior change).

I would feel strange to see the ping span generated if I configured ping_enabled as false (Which also means that where is no way to silence it entirely.)

Agree. I think even document it still confuse people. I suggest either removing the condition of if ping doesn't have parent span ... (i.e. disable all if set false) or make the option name more specific.

cc: @plantfansam


attr_reader :propagator

Expand Down
2 changes: 2 additions & 0 deletions instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
34 changes: 25 additions & 9 deletions instrumentation/trilogy/test/opentelemetry/instrumentation/trilogy/instrumentation_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down