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

Trilogy: instrument connect and ping #704

Merged
merged 1 commit into from
Oct 21, 2023
+63 −8
Merged

Trilogy: instrument connect and ping #704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 31 additions & 7 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 @@ -48,6 +48,28 @@ module Client # rubocop:disable Metrics/ModuleLength

FULL_SQL_REGEXP = Regexp.union(MYSQL_COMPONENTS.map { |component| COMPONENTS_REGEX_MAP[component] })

def initialize(options = {})
@connection_options = options # This is normally done by Trilogy#initialize
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit of a hack, it may make sense to refactor the code upstream a tiny bit to make it less hacky here.

E.g. do the initialization proper in initialize and extract the connection in something like _connect.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine with me. We already have initialize calling _initialize, so if we do the ivar set in Ruby instead we can patch _initialize here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So thanks to you merging trilogy-libraries/trilogy#130, in future versions of trilogy we might be able to just decorate _connect and get rid of this hack.

But overall I don't think it's a big deal, just a bit fragile if upstream refactor things out.


tracer.in_span(
'connect',
attributes: client_attributes.merge!(OpenTelemetry::Instrumentation::Trilogy.attributes),
kind: :client
) do
super
end
end

def ping(...)
tracer.in_span(
'ping',
attributes: client_attributes.merge!(OpenTelemetry::Instrumentation::Trilogy.attributes),
kind: :client
) do
super
end
end

def query(sql)
tracer.in_span(
database_span_name(sql),
Expand All @@ -60,22 +82,24 @@ def query(sql)

private

def client_attributes(sql)
def client_attributes(sql = nil)
attributes = {
::OpenTelemetry::SemanticConventions::Trace::DB_SYSTEM => 'mysql',
::OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME => connection_options.fetch(:host, 'unknown sock')
::OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME => connection_options&.fetch(:host, 'unknown sock') || 'unknown sock'
}

attributes[::OpenTelemetry::SemanticConventions::Trace::DB_NAME] = database_name if database_name
attributes[::OpenTelemetry::SemanticConventions::Trace::DB_USER] = database_user if database_user
attributes[::OpenTelemetry::SemanticConventions::Trace::PEER_SERVICE] = config[:peer_service] unless config[:peer_service].nil?
attributes['db.mysql.instance.address'] = @connected_host if defined?(@connected_host)

case config[:db_statement]
when :obfuscate
attributes[::OpenTelemetry::SemanticConventions::Trace::DB_STATEMENT] = obfuscate_sql(sql)
when :include
attributes[::OpenTelemetry::SemanticConventions::Trace::DB_STATEMENT] = sql
if sql
case config[:db_statement]
when :obfuscate
attributes[::OpenTelemetry::SemanticConventions::Trace::DB_STATEMENT] = obfuscate_sql(sql)
when :include
attributes[::OpenTelemetry::SemanticConventions::Trace::DB_STATEMENT] = sql
end
end

attributes
Expand Down
33 changes: 32 additions & 1 deletion 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 @@ -12,7 +12,7 @@
describe OpenTelemetry::Instrumentation::Trilogy do
let(:instrumentation) { OpenTelemetry::Instrumentation::Trilogy::Instrumentation.instance }
let(:exporter) { EXPORTER }
let(:span) { exporter.finished_spans.first }
let(:span) { exporter.finished_spans[1] }
let(:config) { {} }
let(:driver_options) do
{
Expand Down Expand Up @@ -166,6 +166,37 @@
end
end

describe 'when connecting' do
let(:span) { exporter.finished_spans.first }

it 'spans will include database name' do
_(client.connected_host).wont_be_nil

_(span.name).must_equal 'connect'
_(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.attributes['db.mysql.instance.address']).must_be_nil
end
end

describe 'when pinging' do
let(:span) { exporter.finished_spans[2] }

it 'spans will include database name' do
_(client.connected_host).wont_be_nil

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)
end
end

describe 'when quering for the connected host' do
it 'spans will include the net.peer.name attribute' do
_(client.connected_host).wont_be_nil
Expand Down