feat: typeorm instrumentation #2187
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2187 +/- ##
=======================================
Coverage 90.75% 90.75%
=======================================
Files 157 157
Lines 7734 7734
Branches 1591 1591
=======================================
Hits 7019 7019
Misses 715 715 |
There was a problem hiding this comment.
Thank you for working on this 🙏🎉
Left few comments and suggestions
The original instrumentation had a test-all-versions setup. I wonder if it was left out on purpose or just overlooked.
| import { InstrumentationConfig } from '@opentelemetry/instrumentation'; | ||
|
|
||
| export enum ExtendedDatabaseAttribute { | ||
| DB_STATEMENT_PARAMETERS = 'db.statement.parameters', |
There was a problem hiding this comment.
While this isn't a speced name similar to many others, there is still this rule for semantic attributes which the existing name violates:
Names SHOULD NOT coincide with namespaces. For example if service.instance.id is an attribute name then it is no longer valid to have an attribute named service.instance because service.instance is already a namespace
Since we already have db.statement, I think it is better to use the opportunity and rename it to db.typeorm.parameters (similar to 'db.postgresql.values', 'db.mysql.values'). or some other good name that does not make db.statement a namespace.
| if (typeof statement === 'string') { | ||
| statement = statement.trim(); | ||
| try { | ||
| operation = statement.split(' ')[0].toUpperCase(); |
There was a problem hiding this comment.
The semantic conventions for database says:
[2]: When setting this to an SQL keyword, it is not recommended to attempt any client-side parsing of db.statement just to get this property, but it should be set if the operation name is provided by the library being instrumented
Since this code does not cover many valid cases, can introduce performance hit, and the semantic conventions specifically advise aginst it, I would consider removing it from the instrumentation at the moment, or introducing a config option to opt-in to it.
If we end up keeping it, we can consider refactoring it for better performance - so that we don't need to parse the entire string and create an array just to get the first word:
let firstSpaceIndex = statement.indexOf(' ');
operation = firstSpaceIndex === -1 ? statement.toUpperCase() : statement.substring(0, firstSpaceIndex).toUpperCase();
There was a problem hiding this comment.
Removed it completely for now
specify max version support
Overlooked, going to re-add it, thanks! |
There was a problem hiding this comment.
Thanks for addressing the comments. There are still few threads from previous review I think would be best to address now as this is already a breaking change from the original instrumentation and doing it later will be yet another breaking change for users.
If you prefer that we leave it as is, and address these less trivial cases sometime else, that is also ok with me.
|
@blumamir Anything else to resolve here? |
|
@blumamir Up, could you review I could also help with that as I am waiting for proper typeorm instrumentation. |
Short description of the changes
Adds the typeorm instrumentation created by Aspecto.
Only contains minor changes: upgraded to the latest SDK and semantic conventions package, modified the tests to use
assert, changed the hook type signature.