Skip to content

Conversation

@maryliag
Copy link
Contributor

@maryliag maryliag commented Oct 15, 2024

Which problem is this PR solving?

Short description of the changes

  • Checks if listeners for pg pool events were already added before trying to add them.
  • Generate poolName only once, and use the same on all updates, instead of calculating every time

@maryliag maryliag changed the title not add duplicate listeners to pg pool fix(instrumentation-pg): not add duplicate listeners to pg pool Oct 15, 2024
@codecov
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.75%. Comparing base (b043ffb) to head (64a407f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...elemetry-instrumentation-pg/src/instrumentation.ts 92.30% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2484   +/-   ##
=======================================
  Coverage   90.74%   90.75%           
=======================================
  Files         156      157    +1     
  Lines        7728     7734    +6     
  Branches     1590     1591    +1     
=======================================
+ Hits         7013     7019    +6     
  Misses        715      715           
Files with missing lines Coverage Δ
...telemetry-instrumentation-pg/src/internal-types.ts 100.00% <100.00%> (ø)
...node/opentelemetry-instrumentation-pg/src/utils.ts 97.61% <ø> (-0.02%) ⬇️
...elemetry-instrumentation-pg/src/instrumentation.ts 90.16% <92.30%> (+0.33%) ⬆️

@pichlermarc pichlermarc added bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies labels Oct 16, 2024
Copy link
Member

@pichlermarc pichlermarc 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 taking care of this. 👍
One comment about the added property - otherwise this looks good to be merged.

@pichlermarc pichlermarc linked an issue Oct 16, 2024 that may be closed by this pull request
Copy link
Member

@pichlermarc pichlermarc 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 working on this. There's one more comment I have about he semconv stability flag, once addressed we can merge this.

Copy link
Member

@pichlermarc pichlermarc 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 taking care of this 👍

@pichlermarc pichlermarc merged commit 33c093d into open-telemetry:main Oct 16, 2024
23 checks passed
@dyladan dyladan mentioned this pull request Oct 16, 2024
@maryliag maryliag deleted the pg-memory-leak branch October 16, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working pkg:instrumentation-pg priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory leak in instrumentation-pg

3 participants