-
Notifications
You must be signed in to change notification settings - Fork 549
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
perf(instrumentation-pg): reduce temp objects allocations #2019
Conversation
}); | ||
|
||
span.setAttributes(getSemanticAttributesFromConnection(connectionParameters)); |
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.
note that adding attributes after startSpan
has the side effect that they are not available for a sampler.
Therefore a sampling decision based on e.g. SemanticAttributes.DB_SYSTEM
is no longer possible after this change.
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.
@Flarna oh, makes sense. Guess all is left is to reduce one temp allocation with spreading then, I've updated the MR
b6c0bce
to
dafd1dd
Compare
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.
Thank you for the contribution and for help improving the code.
Added 2 nit comments
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
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2019 +/- ##
==========================================
+ Coverage 90.97% 95.68% +4.70%
==========================================
Files 146 11 -135
Lines 7492 788 -6704
Branches 1502 164 -1338
==========================================
- Hits 6816 754 -6062
+ Misses 676 34 -642 |
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 addressing the previous comments so quickly 🥇
added one last nit comment about the index.ts
Which problem is this PR solving?
The amount of allocations that opentelemetry produces is too high
Short description of the changes
Replaced attributes passed on span creation with span.setAttirbutes to avoid multiple remapping inside startSpan function, reused kind by providing constant and replaced string interpolation with constants