fix: pg span names #1306
fix: pg span names #1306
Conversation
ethanresnick
force-pushed
the
main
branch
3 times, most recently
from
5e6f995 to
6d2dc50
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1306 +/- ##
==========================================
- Coverage 96.08% 95.77% -0.31%
==========================================
Files 14 18 +4
Lines 893 1206 +313
Branches 191 255 +64
==========================================
+ Hits 858 1155 +297
- Misses 35 51 +16
|
|
@rauno56 Re the CI fail: I'm happy to add more tests first before this can be merged, but, before I take the time to do that, please let me know if the PR looks like it's on the right track and is something you think will be accepted. Thanks! |
There was a problem hiding this comment.
Big like on these refactors and the work for improving the span name and error handling.
Added a few minor comments but overall I support these changes.
Your PR makes some changes with no update to existing tests, so I guess the current test coverage is not great. I Would be happy to see some tests to assert the new logic :)
plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
ethanresnick
force-pushed
the
main
branch
2 times, most recently
from
1f619b9 to
b491e1e
Compare
|
@blumamir Ok, I think I've addressed all your comments and have added some tests as well! |
The prior code had three different utility functions for constructing the span to use to trace the query: handleTextQuery, handleConfigQuery, and handleParameterizedQuery. These functions contained a lot of duplicated logic, and that duplication was gonna make it harder to introduce new features (see next commit) and was already leading to bugs/inconsistencies. For example, `handleConfigQuery` would verify that the parameter `values` provided with the query were provided as an array before attaching them to the span, whereas `handleParameterizedQuery` would do so without this validation. This commit instead normalizes the arguments to `client.query` first, so that only function is needed to create the span. In the process of normalizing the arguments, the new code also performs a bit more validation on them, to make sure that the instrumentation will not throw at runtime. For example, the prior code would've thrown on `client.query(null)`, as it was checking `typeof args[0] === 'object'` without excluding null. The prior code also assumed, if an object was passed, that the `text` and `name` keys held a string; this is now verified before attaching that data to the span. Finally, the prior code had two different code paths for invoking the original `client.query` method; one that went through `handleInvalidQuery` and one directly in `_getClientQueryPatch`. The former wrapped the call in a try-catch, while the latter did not. Now, there's only one call to the original client.query, and it's always in a try-catch, which ensures that other cases of invalid args passed to client.query (which might lead it to throw synchronously) won't be an issue.
BREAKING CHANGE. This improves the way that OTEL span names are generated in two different ways. First, if the query is a prepared statement with a `name`, the `name` is put into the span name as-is. Before, the `name` was transformed first, as though it were a full SQL query string (i.e., it was split on spaces and only the first 'word' was used). Second, when we do only have a full query string, and we take the first word to form the span name, the code now calls `toUpperCase()` on that string, so that a `select * from x` and a `SELECT * from x` query both end up with a span name like `pg.query:SELECT`.
|
@blumamir Thanks for the quick + detailed feedback! I added a couple more test cases per your comments. Is this good to merge now, or does it need a second reviewer? |
Thanks again for the contribution and great improvements 🥇 |
|
I don't want to be the blocker and don't have cycles to review it this week. Can merge if we have one pair of maintainer eyes seen and approved it. |
|
Thanks @rauno56! |
Which problem is this PR solving?
The primary functional change in this PR is to address a number of issues with how the pg instrumentation was generating span names.
Before this PR, when the query being executed was a prepared statement with a
name, thenamewas transformed first before it was put into the span's name. E.g.,client.query({ name: 'find user', text: 'SELECT * from user where id = ?', values: [1] })would generate a span name ofpg.query:find, because the string'find user'would be split on spaces and only the first word would be used. However, it's the full prepared statement name that constitutes the low cardinality identifier, so this commit now makes the span name:pg.query:find user.Similarly, before this PR, the parsed out operation name wasn't normalized before using it in the span name. Accordingly, a
select * from xwould get a span name ofpg.query:select, while aSELECT * from xquery would generatepg.query:SELECT. Now, they'll both have the latter span name.Previously, the generated span names did not include the DB name, although the examples in the otel spec recommend this. Now, the db name is included, as it's specified in the connection info.
There are also two commits prior to the commit that adjusts the span naming strategy. Those commits do not make any functional changes, but just refactor/DRY up the existing code, which allows the new span names to be constructed in only one place. They also make the logic a bit more defensive, so that various edge cases with invalid arguments (e.g.,
client.query(null)orclient.query(queryText, valuesThatArentAnArray)) can no longer cause the instrumentation itself to fail with an uncaught exception (though errors raised synchronously or asynchronously fromclient.queryare still propagated as before).Short description of the changes
See each commit message and the overview above.