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

Jump to bottom

Feat: Add Mysql2 and Trilogy db.collection.name attribute #1109

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Feat: Add Mysql2 and Trilogy db.collection.name attribute #1109

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
43d24ee
Add Mysql2 and Trilogy collection_name attribute
hannahramadan Aug 8, 2024
8bd5818
Remove empty line
hannahramadan Aug 8, 2024
146bd71
Appease rubocop
hannahramadan Aug 8, 2024
f9c244e
Refactor compact!
hannahramadan Aug 20, 2024
6047052
Apply suggestions from code review
hannahramadan Aug 20, 2024
cc43b33
Merge branch 'main' into mysql_libs_db_collection_name
kaylareopelle Aug 20, 2024
b61e51e
Update to older semantic convention
hannahramadan Aug 21, 2024
bc7df18
Merge branch 'mysql_libs_db_collection_name' of https://github.com/ha…
hannahramadan Aug 21, 2024
a23b954
Update regex
hannahramadan Aug 26, 2024
f099af3
Use correct variable
hannahramadan Aug 26, 2024
8134ee3
Go directly to matching data vs MatchData object
hannahramadan Aug 26, 2024
8328668
Feature flag
hannahramadan Aug 27, 2024
8f42fa2
No nils
hannahramadan Aug 27, 2024
e73b23a
Omit db_sql_table by default
hannahramadan Oct 14, 2024
46616da
Capture table names in double quotes
hannahramadan Oct 14, 2024
4af3e2c
Don't report table name on error
hannahramadan Oct 30, 2024
d6ffcdf
Apply suggestions from code review
hannahramadan Nov 8, 2024
d9f1975
Update regex
hannahramadan Nov 8, 2024
b8d5a96
Merge branch 'mysql_libs_db_collection_name' of https://github.com/ha…
hannahramadan Nov 8, 2024
68f42ec
Only make one sql table method call
hannahramadan Nov 8, 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
3 changes: 3 additions & 0 deletions instrumentation/mysql2/example/mysql2.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,6 @@
client.query("SELECT * from information_schema.INNODB_TABLES; /**Dé**/").each do |row|
puts row
end

client.query('CREATE TABLE test_table (id SERIAL PRIMARY KEY, name VARCHAR(50), age INT)')
client.query('DROP TABLE test_table')
15 changes: 13 additions & 2 deletions instrumentation/mysql2/lib/opentelemetry/instrumentation/mysql2/patches/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ module Mysql2
module Patches
# Module to prepend to Mysql2::Client for instrumentation
module Client
# Capture the first word (including letters, digits, underscores, & '.', ) that follows common table commands
TABLE_NAME = /\b(?:FROM|INTO|UPDATE|CREATE\s+TABLE(?:\s+IF\s+NOT\s+EXISTS)?|DROP\s+TABLE(?:\s+IF\s+EXISTS)?|ALTER\s+TABLE(?:\s+IF\s+EXISTS)?)\s+([\w\.]+)/i

def query(sql, options = {})
tracer.in_span(
_otel_span_name(sql),
Expand Down Expand Up @@ -47,7 +50,7 @@ def _otel_span_name(sql)
end

def _otel_span_attributes(sql)
attributes = _otel_client_attributes
attributes = _otel_client_attributes(sql)
case config[:db_statement]
when :include
attributes[SemanticConventions::Trace::DB_STATEMENT] = sql
Expand All @@ -68,7 +71,7 @@ def _otel_database_name
(query_options[:database] || query_options[:dbname] || query_options[:db])&.to_s
end

def _otel_client_attributes
def _otel_client_attributes(sql)
# The client specific attributes can be found via the query_options instance variable
# exposed on the mysql2 Client
# https://github.com/brianmario/mysql2/blob/ca08712c6c8ea672df658bb25b931fea22555f27/lib/mysql2/client.rb#L25-L26
Expand All @@ -83,9 +86,17 @@ def _otel_client_attributes

attributes[SemanticConventions::Trace::DB_NAME] = _otel_database_name
attributes[SemanticConventions::Trace::PEER_SERVICE] = config[:peer_service]
attributes['db.collection.name'] = collection_name(sql)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to avoid adding additional overhead to the instrumentation if possible.

In cases where the SQL statement is omitted or not sanitized, there isn't any regexp scan that occurs.

Is there a way to optimize it so that there isn't additional processing?

I would also like to consider that this db.collection.name matches newer versions of the OTel Schema and the instrumentations are still using pre-1.0 semantics.

If I am not mistaken that would have been db.table.name. I think we should continue using pre-1.0 semantics until we have
OTEL_SEMCONV_STABILITY_OPT_IN implemented

https://opentelemetry.io/docs/specs/semconv/database/database-spans/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @arielvalentin! It makes sense to keep consistent with pre-1.0 semantics. In this case, the attribute is db.sql.table. I've made this update!

Re additional processing: I see your point about putting every SQL statement through a regexp scan. Because the table/collection name isn't available on the client, I'm not sure how else we'd be able to get this information. We could make table name an opt-in/opt-out attribute, but is that level of control something we want to provide? What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean it's difficult to say. I feel like maybe this should only be done when the we also include the db.statement and I don't know if it would be possible to combine with the obfuscation code.

Maybe I'm making too much of it? I'm not sure.

Perhaps benchmarking will put me a bit at ease.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if only including the table name if we're recording the db.statement deviates from the convention (because as of now, the convention has it that recording the collection name isn't based on any condition, just availability).

The change to Regexp.last_match(1) and an updated regex string improved speed. In the following benchmark example, I used the MySQL obfuscation SQL code as a baseline, and found running the extra db.collection.name sql was 1.19x slower.

require 'benchmark/ipsa'

TABLE_NAME = /\b(?:(?:FROM|INTO|UPDATE)|(?:(?:CREATE|DROP|ALTER)\s+TABLE(?:\s+IF\s+(?:NOT\s+)?EXISTS)?))\s+["']?([\w.]+)["']?/i
MYSQL_OBFUSCATION = /(?-mix:'(?:[^']|'')*?(?:\\'.*|'(?!')))|(?-mix:"(?:[^"]|"")*?(?:\\".*|"(?!")))|(?-mix:(\$(?!\d)[^$]*?\$).*?(?:\1|$))|(?-mix:\{?(?:[0-9a-fA-F]\-*){32}\}?)|(?-mix:-?\b(?:[0-9]+\.)?[0-9]+([eE][+-]?[0-9]+)?\b)|(?i-mx:\b(?:true|false|null)\b)|(?-mix:0x[0-9a-fA-F]+)|(?i-mx:(?:#|--).*?(?=\r|\n|$))|(?m-ix:\/\*.*?\*\/)|(?-mix:q'\[.*?(?:\]'|$)|q'\{.*?(?:\}'|$)|q'\<.*?(?:\>'|$)|q'\(.*?(?:\)'|$))/
SQL = 'SELECT * FROM test_table'

def collection_name
  shared_operation
  Regexp.last_match(1) if SQL =~ TABLE_NAME
end

def no_collection_name
  shared_operation
  SQL
end

def shared_operation
  SQL.gsub(MYSQL_OBFUSCATION, '?')
end

Benchmark.ipsa do |x|
  x.report('collection_name') { collection_name }
  x.report('no_collection_name') { no_collection_name }

  x.compare!
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arielvalentin We chatted about this in the SIG today and decided the reporting of table/collection names can be put behind a feature flag. What do you think? If that works, the remaining consideration is if the default on or off - do you have any preference on this?

Copy link
Contributor Author

@hannahramadan hannahramadan Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@arielvalentin - I made a suggestion for the config name db_collection_name and default value include, with omit as the other option: 8328668

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @arielvalentin! Wanted to check in and see if you had thoughts on the above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for your patience. I'm going to add this to my list to review by EoD tomorrow


attributes
end

def collection_name(sql)
sql.scan(TABLE_NAME).flatten[0]
rescue StandardError
hannahramadan marked this conversation as resolved.
Show resolved Hide resolved
nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

If an error occurs and the attribute is set to nil, then the SDK will report an error. It would be best to avoid setting any invalid attributes.

Please ensure that the attribute is not set in cases where the table name could not be extracted.

https://github.com/open-telemetry/opentelemetry-ruby/blob/555b062ef9421784c132aa9b97b29ec637b13b0f/sdk/lib/opentelemetry/sdk/trace/span.rb#L277

https://github.com/open-telemetry/opentelemetry-ruby/blob/555b062ef9421784c132aa9b97b29ec637b13b0f/sdk/lib/opentelemetry/sdk/internal.rb#L57

Copy link
Contributor Author

@hannahramadan hannahramadan Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Made some changes! Will log an error if trouble getting the table name and won't report. Is the error logging here okay? 4af3e2c

end

def tracer
Mysql2::Instrumentation.instance.tracer
end
Expand Down
54 changes: 54 additions & 0 deletions instrumentation/mysql2/test/fixtures/sql_table_name.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
[
{
"name": "from",
"sql": "SELECT * FROM test_table"
},
{
"name": "select_count_from",
"sql": "SELECT COUNT(*) FROM test_table WHERE condition"
},
{
"name": "from_with_subquery",
"sql": "SELECT * FROM (SELECT * FROM test_table) AS table_alias"
},
{
"name": "insert_into",
"sql": "INSERT INTO test_table (column1, column2) VALUES (value1, value2)"
},
{
"name": "update",
"sql": "UPDATE test_table SET column1 = value1 WHERE condition"
},
{
"name": "delete_from",
"sql": "DELETE FROM test_table WHERE condition"
},
{
"name": "create_table",
"sql": "CREATE TABLE test_table (column1 datatype, column2 datatype)"
},
{
"name": "create_table_if_not_exists",
"sql": "CREATE TABLE IF NOT EXISTS test_table (column1 datatype, column2 datatype)"
},
{
"name": "alter_table",
"sql": "ALTER TABLE test_table ADD column_name datatype"
},
{
"name": "drop_table",
"sql": "DROP TABLE test_table"
},
{
"name": "drop_table_if_exists",
"sql": "DROP TABLE IF EXISTS test_table"
},
{
"name": "insert_into",
"sql": "INSERT INTO test_table values('', 'a''b c',0, 1 , 'd''e f''s h')"
},
{
"name": "from_with_join",
"sql": "SELECT columns FROM test_table JOIN table2 ON test_table.column = table2.column"
}
]
22 changes: 20 additions & 2 deletions instrumentation/mysql2/test/opentelemetry/instrumentation/mysql2/instrumentation_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
# 1. Build the opentelemetry/opentelemetry-ruby-contrib image
# - docker-compose build
# 2. Bundle install
# - docker-compose run ex-instrumentation-mysql2-test bundle install
# - docker-compose run ex-instrumentation-mysql2-test bundle exec appraisal install
# 3. Run test suite
# - docker-compose run ex-instrumentation-mysql2-test bundle exec rake test
# - docker-compose run ex-instrumentation-mysql2-test bundle exec appraisal rake test
describe OpenTelemetry::Instrumentation::Mysql2::Instrumentation do
let(:instrumentation) { OpenTelemetry::Instrumentation::Mysql2::Instrumentation.instance }
let(:exporter) { EXPORTER }
Expand Down Expand Up @@ -473,6 +473,24 @@
end
end
end

describe '#connection_name' do
def self.load_fixture
data = File.read("#{Dir.pwd}/test/fixtures/sql_table_name.json")
JSON.parse(data)
end

load_fixture.each do |test_case|
name = test_case['name']
query = test_case['sql']

it "returns the table name for #{name}" do
table_name = client.send(:collection_name, query)

expect(table_name).must_equal('test_table')
end
end
end
end
end unless ENV['OMIT_SERVICES']
end
13 changes: 12 additions & 1 deletion 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 @@ -13,6 +13,9 @@ module Trilogy
module Patches
# Module to prepend to Trilogy for instrumentation
module Client
# Capture the first word (including letters, digits, underscores, & '.', ) that follows common table commands
TABLE_NAME = /\b(?:FROM|INTO|UPDATE|CREATE\s+TABLE(?:\s+IF\s+NOT\s+EXISTS)?|DROP\s+TABLE(?:\s+IF\s+EXISTS)?|ALTER\s+TABLE(?:\s+IF\s+EXISTS)?)\s+([\w\.]+)/i

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

Expand Down Expand Up @@ -76,6 +79,8 @@ def client_attributes(sql = nil)
attributes['db.instance.id'] = @connected_host unless @connected_host.nil?

if sql
attributes['db.collection.name'] = collection_name(sql)

case config[:db_statement]
when :obfuscate
attributes[::OpenTelemetry::SemanticConventions::Trace::DB_STATEMENT] =
Expand All @@ -84,10 +89,16 @@ def client_attributes(sql = nil)
attributes[::OpenTelemetry::SemanticConventions::Trace::DB_STATEMENT] = sql
end
end

attributes.compact!
hannahramadan marked this conversation as resolved.
Show resolved Hide resolved
attributes
end

def collection_name(sql)
sql.scan(TABLE_NAME).flatten[0]
rescue StandardError
nil
end

def database_name
connection_options[:database]
end
Expand Down
54 changes: 54 additions & 0 deletions instrumentation/trilogy/test/fixtures/sql_table_name.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
[
{
"name": "from",
"sql": "SELECT * FROM test_table"
},
{
"name": "select_count_from",
"sql": "SELECT COUNT(*) FROM test_table WHERE condition"
},
{
"name": "from_with_subquery",
"sql": "SELECT * FROM (SELECT * FROM test_table) AS table_alias"
},
{
"name": "insert_into",
"sql": "INSERT INTO test_table (column1, column2) VALUES (value1, value2)"
},
{
"name": "update",
"sql": "UPDATE test_table SET column1 = value1 WHERE condition"
},
{
"name": "delete_from",
"sql": "DELETE FROM test_table WHERE condition"
},
{
"name": "create_table",
"sql": "CREATE TABLE test_table (column1 datatype, column2 datatype)"
},
{
"name": "create_table_if_not_exists",
"sql": "CREATE TABLE IF NOT EXISTS test_table (column1 datatype, column2 datatype)"
},
{
"name": "alter_table",
"sql": "ALTER TABLE test_table ADD column_name datatype"
},
{
"name": "drop_table",
"sql": "DROP TABLE test_table"
},
{
"name": "drop_table_if_exists",
"sql": "DROP TABLE IF EXISTS test_table"
},
{
"name": "insert_into",
"sql": "INSERT INTO test_table values('', 'a''b c',0, 1 , 'd''e f''s h')"
},
{
"name": "from_with_join",
"sql": "SELECT columns FROM test_table JOIN table2 ON test_table.column = table2.column"
}
]
28 changes: 28 additions & 0 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 @@ -9,6 +9,16 @@
require_relative '../../../../lib/opentelemetry/instrumentation/trilogy'
require_relative '../../../../lib/opentelemetry/instrumentation/trilogy/patches/client'

# This test suite requires a running mysql container and dedicated test container. We can use the same
hannahramadan marked this conversation as resolved.
Show resolved Hide resolved
# docker-compose file as the mysql2 instrumentation tests. The test container should have the mysql client.
hannahramadan marked this conversation as resolved.
Show resolved Hide resolved
# To run tests locally:
# 1. Build the opentelemetry/opentelemetry-ruby-contrib image
# - docker-compose build
# 2. Open a bash shell in the test container and cd to the trilogy directory
# - docker-compose run ex-instrumentation-mysql2-test bash -c 'cd ../trilogy && bash'
# 3. Bundle install and run tests with the Appraisals gem
# - bundle exec appraisal install && bundle exec appraisal rake test

describe OpenTelemetry::Instrumentation::Trilogy do
let(:instrumentation) { OpenTelemetry::Instrumentation::Trilogy::Instrumentation.instance }
let(:exporter) { EXPORTER }
Expand Down Expand Up @@ -626,5 +636,23 @@
end
end
end

describe '#connection_name' do
def self.load_fixture
data = File.read("#{Dir.pwd}/test/fixtures/sql_table_name.json")
JSON.parse(data)
end

load_fixture.each do |test_case|
name = test_case['name']
query = test_case['sql']

it "returns the table name for #{name}" do
table_name = client.send(:collection_name, query)

expect(table_name).must_equal('test_table')
end
end
end
end
end
Loading