fix: Ensure encoding errors handled during SQL obfuscation for Trilogy #345
Conversation
This file contains hidden or 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 should be supported in the same way as Mysql2's implementation.
…ilogy instrumentation
adrianna-chang-shopify
requested review from
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
mwear,
plantfansam,
robbkidd and
robertlaurin
as code owners
Contributor
|
Hey @adrianna-chang-shopify just ack'ing this. This looks good to me but i'll admit i'm very out of touch with Trilogy at the moment, and would appreciate input from @arielvalentin on this. I will try to give this a closer look this evening but seems straightforward imo and appreciate you keeping this instrumentation in line with mysql2 |
arielvalentin
approved these changes
Feb 27, 2023
instrumentation/mysql2/test/opentelemetry/instrumentation/mysql2/instrumentation_test.rb
Show resolved
Hide resolved
instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb
Show resolved
Hide resolved
instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb
Outdated
Show resolved
Hide resolved
ericmustin
approved these changes
Feb 27, 2023
Contributor
ericmustin
left a comment
ericmustin
left a comment
There was a problem hiding this comment.
lgtm pending @arielvalentin suggested change wrt error handling
instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb
Outdated
Show resolved
Hide resolved
instrumentation/mysql2/lib/opentelemetry/instrumentation/mysql2/patches/client.rb
Outdated
Show resolved
Hide resolved
adrianna-chang-shopify
force-pushed
the
ac-trilogy-encoding-errors
branch
from
c82d921 to
deaf871
Compare
adrianna-chang-shopify
force-pushed
the
ac-trilogy-encoding-errors
branch
from
deaf871 to
d4a33e0
Compare
Contributor
Author
|
Oops let me just update the CHANGELOG quickly |
Contributor
The deploy robot will handle that for us, but if you've already updated it that's ok! |
arielvalentin
requested changes
Feb 27, 2023
adrianna-chang-shopify
force-pushed
the
ac-trilogy-encoding-errors
branch
from
b4fb2c9 to
d4a33e0
Compare
adrianna-chang-shopify
changed the title
arielvalentin
approved these changes
Feb 27, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Shopify is planning to start migrating apps from Mysql2 to Trilogy as a SQL database client. Ensuring parity between OpenTelemetry instrumentation for Mysql2 and Trilogy is important before we move to Trilogy.
This PR ensures that invalid byte sequences are handled appropriately when obfuscating SQL, taking inspiration from #160. I also added some additional tests to the Trilogy spec around configuring
db_statementvia an environment variable.I noticed the Mysql2 test around invalid byte encoding was removed in #265 when the
enable_sql_obfuscationconfig went away, and I'm not sure if that was intentional. I've added a test to the Mysql2 spec to ensure there's coverage for an invalid byte sequence appearing in SQL.