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

update bluebird instrumentation to account for getNewLibraryCopy #813

Merged
merged 18 commits into from
Jan 20, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
c20af16
handle usage of getNewLibraryCopy by sequelize
ericmustin Dec 27, 2019
8165515
[wip] add promise spec for getNewLibraryCopy need to determine min ve…
ericmustin Dec 27, 2019
a5f4159
fix unwrap typo, add temp fix and todos for version requirements for …
ericmustin Dec 28, 2019
ec3a81c
add getNewLibraryCopy patching only for versions where it exists
ericmustin Dec 29, 2019
98320d7
handle unwrapping copies _then by maintaining reference to copies on …
ericmustin Dec 31, 2019
9bd9ea6
make hidden property non enumerable
ericmustin Dec 31, 2019
11d627c
[wip] add specs for patching and unpatching behavior , clean up hidde…
ericmustin Jan 1, 2020
0838c00
add specs for mechanics of tracking wrapped bluebird library copies f…
ericmustin Jan 1, 2020
9297097
remove duplicate patching specs
ericmustin Jan 2, 2020
2d6860d
add version filter for bluebird version specific instrumentation
ericmustin Jan 7, 2020
f2a97d2
generalize unpatching test for all promise libraries
ericmustin Jan 7, 2020
5d26c79
handle edge case when non native promise libraries default to native …
ericmustin Jan 7, 2020
2cc45bb
only instrument promisejs when it doesnt export global promise, move …
ericmustin Jan 7, 2020
361c6d0
only instrument promise-js on versions it isnt used native promises, …
ericmustin Jan 7, 2020
29152ed
simplify version checking and test pre / post patching and unpatching…
ericmustin Jan 10, 2020
afdf878
es6 syntax improvement
ericmustin Jan 10, 2020
7f834c2
fix beforeEach handling of promises
ericmustin Jan 10, 2020
99c0518
add test for copy of copy being unwrapped
ericmustin Jan 16, 2020
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
51 changes: 51 additions & 0 deletions packages/datadog-plugin-bluebird/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,58 @@

const tx = require('../../dd-trace/src/plugins/util/promise')

const DD_LIB_COPIES = '_datadog_library_copies'

function createGetNewLibraryCopyWrap (tracer, config, originalLib, shim) {
return function wrapGetNewLibraryCopy (getNewLibraryCopy) {
return function getNewLibraryCopyWithTrace () {
const libraryCopy = getNewLibraryCopy.apply(this, arguments)
shim.wrap(libraryCopy.prototype, '_then', tx.createWrapThen(tracer, config))
addToLibraryCopies(originalLib.prototype, libraryCopy)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any particular reason to stash this on the prototype rather than the class/constructor? Just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, no good reason, updated to point to the constructor instead

return libraryCopy
}
}
}

function addToLibraryCopies (originalLibPrototype, libraryCopy) {
let libraryCopies = originalLibPrototype[DD_LIB_COPIES]

if (!libraryCopies) {
libraryCopies = new Set()

Object.defineProperty(originalLibPrototype, DD_LIB_COPIES, {
enumerable: false,
writable: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can set the value here rather than on line 29 with value: libraryCopies.

You should also add configurable: true.

enumerable: false is the default, so you can omit it if you want. (Your choice.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, updated with configurable and value and using the enumerable default

})

originalLibPrototype[DD_LIB_COPIES] = libraryCopies
}

libraryCopies.add(libraryCopy)
}

function unwrapLibraryCopies (originalLibPrototype, shim) {
const libraryCopies = originalLibPrototype[DD_LIB_COPIES]

if (libraryCopies) {
libraryCopies.forEach(libraryCopy => shim.unwrap(libraryCopy.prototype, '_then'))
libraryCopies.clear()
delete originalLibPrototype[DD_LIB_COPIES]
}
}

module.exports = [
{
name: 'bluebird',
versions: ['^2.11.0', '^3.4.1'],
patch (Promise, tracer, config) {
this.wrap(Promise, 'getNewLibraryCopy', createGetNewLibraryCopyWrap(tracer, config, Promise, this))
},
unpatch (Promise) {
this.unwrap(Promise, 'getNewLibraryCopy')
unwrapLibraryCopies(Promise.prototype, this)
}
},
{
name: 'bluebird',
versions: ['>=2.0.2'], // 2.0.0 and 2.0.1 were removed from npm
Expand Down
11 changes: 11 additions & 0 deletions packages/datadog-plugin-bluebird/test/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,14 @@
const assertPromise = require('../../dd-trace/test/plugins/promise')

assertPromise('bluebird')

assertPromise('bluebird', bluebird => {
// TODO: remove if statement when running tests only for versions ^2.11.0 and ^3.4.1
rochdev marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/petkaantonov/bluebird/releases/tag/v2.11.0
// https://github.com/petkaantonov/bluebird/releases/tag/v3.4.1
if (!bluebird.getNewLibraryCopy) {
return bluebird
}

return bluebird.getNewLibraryCopy()
})