Skip to content
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

Merged
merged 4 commits into from
Mar 25, 2024

Conversation

Samuron
Copy link
Contributor

@Samuron Samuron commented Mar 18, 2024

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

});

span.setAttributes(getSemanticAttributesFromConnection(connectionParameters));
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

@blumamir blumamir left a 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

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Merging #2019 (53c2f8f) into main (dfb2dff) will increase coverage by 4.70%.
Report is 15 commits behind head on main.
The diff coverage is n/a.

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     

see 136 files with indirect coverage changes

Copy link
Member

@blumamir blumamir left a 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

plugins/node/opentelemetry-instrumentation-pg/src/index.ts Outdated Show resolved Hide resolved
@pichlermarc pichlermarc merged commit 84ef980 into open-telemetry:main Mar 25, 2024
17 checks passed
@dyladan dyladan mentioned this pull request Mar 25, 2024
@Samuron Samuron deleted the pg_reduce_alloc branch March 25, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants