-
Notifications
You must be signed in to change notification settings - Fork 169
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.
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: Add Mysql2 and Trilogy db.collection.name
attribute
#1109
Open
hannahramadan
wants to merge
15
commits into
open-telemetry:main
Choose a base branch
from
hannahramadan:mysql_libs_db_collection_name
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
43d24ee
Add Mysql2 and Trilogy collection_name attribute
hannahramadan 8bd5818
Remove empty line
hannahramadan 146bd71
Appease rubocop
hannahramadan f9c244e
Refactor compact!
hannahramadan 6047052
Apply suggestions from code review
hannahramadan cc43b33
Merge branch 'main' into mysql_libs_db_collection_name
kaylareopelle b61e51e
Update to older semantic convention
hannahramadan bc7df18
Merge branch 'mysql_libs_db_collection_name' of https://github.com/ha…
hannahramadan a23b954
Update regex
hannahramadan f099af3
Use correct variable
hannahramadan 8134ee3
Go directly to matching data vs MatchData object
hannahramadan 8328668
Feature flag
hannahramadan 8f42fa2
No nils
hannahramadan e73b23a
Omit db_sql_table by default
hannahramadan 46616da
Capture table names in double quotes
hannahramadan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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" | ||
} | ||
] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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" | ||
} | ||
] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 haveOTEL_SEMCONV_STABILITY_OPT_IN
implementedhttps://opentelemetry.io/docs/specs/semconv/database/database-spans/
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 extradb.collection.name
sql was 1.19x slower.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 valueinclude
, withomit
as the other option: 8328668There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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