-
Notifications
You must be signed in to change notification settings - Fork 507
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
refactor(instrumentation-mysql2): improve performance of getSpanName using substring #2470
base: main
Are you sure you want to change the base?
refactor(instrumentation-mysql2): improve performance of getSpanName using substring #2470
Conversation
Would you mind doing the same change on the other mysql package, to keep consistency: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-mysql/src/utils.ts#L118 |
This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2470 +/- ##
==========================================
- Coverage 90.74% 90.53% -0.21%
==========================================
Files 156 99 -57
Lines 7723 5092 -2631
Branches 1588 1008 -580
==========================================
- Hits 7008 4610 -2398
+ Misses 715 482 -233
|
Thanks for contributing to OpenTelemetry :) Could you run |
thanks for the review @maryliag I implemented the change in instrumentation-mysql, but it was slightly different as it was returning the full query string in case of an object instead of just the verb. I'm not sure wether it's safe or not to apply this change as a test was failing. I also fixed the lint issues, sorry about that @david-luna |
81e955b
to
ddd8401
Compare
Thank you for updating the other package too! 😄 |
Which problem is this PR solving?
We noticed that
getSpanName()
frominstrumentation-mysql2
was taking non-negligeable time from our profiler in production, considering that it's in a critical path.This PR replaces split() by substring() to avoid instanciating a new array everytime.
Here is a benchmark with the difference before and after (~20 times faster for me): https://jsbm.dev/YtumA6nRNvjVJ
Short description of the changes
Use substring() with indexOf() instead of split() to extract the SQL verb from a sql query. It also checks for queries with no spaces like "COMMIT" and fallback to return the raw query.