Skip to content

fix: memory leak in @elastic/elasticsearch instrumentation #2571

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 11, 2022
Merged

fix: memory leak in @elastic/elasticsearch instrumentation #2571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d52c53a
fix: memory leak in @elastic/elasticsearch instrumentation
trentm Feb 10, 2022
043dca0
Merge branch 'main' into trentm/issue2569-memleak2
trentm Feb 10, 2022
cc6891a
correctly .get from WeakMaps, duh
trentm Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,28 @@ Notes:
[[release-notes-3.x]]
=== Node.js Agent version 3.x

==== Unreleased

[float]
===== Breaking changes

[float]
===== Features

[float]
===== Bug fixes

* Fix a bug in instrumentation of `@elastic/elasticsearch` that caused a
memory leak. ({issues}2569[#2569])


[[release-notes-3.28.0]]
==== 3.28.0 2022/02/08

Known issue: This release includes a memory leak in instrumentation of the
`@elastic/elasticsearch` package. If you use that package, you should not
use v3.28.0 of this APM agent. ({issues}2569[#2569])

[float]
===== Breaking changes

Expand Down
10 changes: 5 additions & 5 deletions lib/instrumentation/modules/@elastic/elasticsearch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ module.exports = function (elasticsearch, agent, { version, enabled }) {
if (result) {
const currSpan = ins.currSpan()
if (currSpan) {
diagResultFromSpan[currSpan] = result
diagResultFromSpan.set(currSpan, result)
Copy link
Member Author

@trentm trentm Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:embarrassment:

Copy link
Contributor

@astorm astorm Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it got by my review too so sign me up for the :embarrassment: newsletter.

Copy link
Member

@felixbarny felixbarny Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that's a pitfall if I have ever seen one.

Copy link
Member Author

@trentm trentm Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dynamic languages for you. Not sure if TypeScript would have caught this for me.

}
}
})
Expand All @@ -91,7 +91,7 @@ module.exports = function (elasticsearch, agent, { version, enabled }) {
const conn = origGetConnection.apply(this, arguments)
const currSpan = ins.currSpan()
if (conn && currSpan) {
connFromSpan[currSpan] = conn
connFromSpan.set(currSpan, conn)
}
return conn
}
Expand Down Expand Up @@ -136,7 +136,7 @@ module.exports = function (elasticsearch, agent, { version, enabled }) {
// Otherwise, fallback to using the first connection on
// `Transport#connectionPool`, if any. (This is the best parsed
// representation of connection options passed to the Client ctor.)
let conn = connFromSpan[span]
let conn = connFromSpan.get(span)
if (conn) {
connFromSpan.delete(span)
} else if (this.connectionPool && this.connectionPool.connections) {
Expand All @@ -154,7 +154,7 @@ module.exports = function (elasticsearch, agent, { version, enabled }) {
// content-length: ...
//
// Getting the ES client request "DiagnosticResult" object has some edge cases:
// - In v7 using a callback, we always get `result`.
// - In v7 using a callback, we always get it as `result`.
// - In v7 using a Promise, if the promise is rejected, then `result` is
// not passed.
// - In v8, `result` only includes HTTP response info if `options.meta`
Expand All @@ -166,7 +166,7 @@ module.exports = function (elasticsearch, agent, { version, enabled }) {
// `diagResult`.
let diagResult = isGteV8 ? null : result
if (!diagResult) {
diagResult = diagResultFromSpan[span]
diagResult = diagResultFromSpan.get(span)
if (diagResult) {
diagResultFromSpan.delete(span)
}
Expand Down