update bluebird instrumentation to account for getNewLibraryCopy

[ericmustin]

What does this PR do?

The Bluebird promise library added a helper function getNewLibraryCopy in versions 2.11.0 and 3.4.1 which:

Returns a new independent copy of the Bluebird library.

Usage of this helper method in turn causes the dd-trace-js bluebird instrumentation to get dropped. This PR adds instrumentation to ensure that independant copy of the bluebird library is also instrumented.

Motivation

This helper method is used by the popular Node ORM Sequelize, seen here. As a result, if a basic express/sequelize/(pg|mysql) MVC application receives concurrent requests, dd-trace-js instrumentation attaches the database spans to the incorrect trace. You can see a quick screenshot of this behavior, and I'm including a link to a basic express/sequelize application and a script for a trivial reproduction of the issue below. Sorry the repro is a bit hacky, it's quite easy to reproduce though if you want to test however you prefer:

Reproduction of previous behavior:

1. Run express/sequelize application: https://github.com/ericmustin/sample_sequelize_express_app

2. with the application listening on localhost:3000 run test script for making concurrent requests:
   https://gist.github.com/ericmustin/d70dfc9fdfb78732302a119793ad7c44

3. Output:
   

And, with this update applied this is handled correctly:

Plugin Checklist

• [x ] Unit tests.
• TypeScript definitions.
• TypeScript tests.
• API documentation.
• CircleCI jobs/workflows.
• Plugin is exported.

Additional Notes

A few things. First is that this helper method wasn't added for v2 and v3 until 2.11.0 and 3.4.1, respectively (i've linked to the release notes in the previous section). However I wasn't entirely sure how to handle the versioning bit in datadog-plugin-bluebird/src/index.js and updating the dd-trace-js/versions (this is auto generated by yarn, ignore) directory. My thinking for the instrumentation was adding additional objects to the module exports ala

  ....
  {
    name: 'bluebird',
    versions: ['^3.4.1']
    patch (Promise, tracer, config) {
      if(Promise.getNewLibraryCopy) {
        this.wrap(Promise, 'getNewLibraryCopy', createGetNewLibraryCopyWrap(tracer, config, this))  
      }
    },
    unpatch (Promise) {
      if(Promise.getNewLibraryCopy) {
        this.unwrap(Promise, 'getNewLibraryCopy')
      }
    }
  },
  {
    name: 'bluebird',
    versions: ['^2.11.0']
    patch (Promise, tracer, config) {
      if(Promise.getNewLibraryCopy) {
        this.wrap(Promise, 'getNewLibraryCopy', createGetNewLibraryCopyWrap(tracer, config, this))  
      }
    },
    unpatch (Promise) {
      if(Promise.getNewLibraryCopy) {
        this.unwrap(Promise, 'getNewLibraryCopy')
      }
    }
  }  

But I wasn't entirely if these are incremental or not (ie, should I do all the wrapping in here or just the bit that applies to ^2.11.0 / ^3.4.1? And also is there documentation on how those details in the versions folder are generated? (auto generated by yarn, ignore)

In the interim I've just added some defensive code in both the unit tests and the instrumentation code to ensure that we only attempt to patch when this function exists. Any guidance here on the proper way to do this and how to generate the right files in the versions directory ( is there some stuff in the scripts directory I run?) would be much appreciated.

Also just generally let me know if my approach here for accounting for this helper function in bluebird is appropriate. Although this is meant to get sequelize working, as I assume it's a relatively widely used helper function I kept all the instrumentation as part of core bluebird and not something sequelize specific. Lmk if that is way off. This update works for me as far as fixing database spans getting assigned to the wrong trace, but not sure if there's a more canonical way to instrument this sort of helper method.

thanks for taking a look!

[ericmustin]

just to update, I've gone ahead and added what I think is the right way to do versioning for the plugin, by adding this additional object to the module.exports array. And i think i answered my own question on the versions/ directory which is that yarn handles that bit

  {
    name: 'bluebird',
    versions: ['^2.11.0', '^3.4.1'],
    patch: <...patching code>,
    unpatch: <...unpatching code>
  },
...

Anyway lmk what you think! cheers

[ericmustin]

just a quick update, spoke informally about this with @bengl , he pointed out that this had originally failed to unpatch the library copies generated by getNewLibraryCopy. Which presents a bit of a bind as there is no canonical place to store this sort of reference to a dynamically generated library copy for later unpatching. I tried to address this with a hidden property on the original library and the helper methods addToLibraryCopies and unwrapLibraryCopies to push wrapped copies of the library during patching, and then iterate/unwrap them during unpatching. Let me know what you think here of this approach

[bengl]

One small change requested. Other than that, looks good.

Also, this functionality is sufficiently complicated that it warrants a test.

[bengl]

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.)

[ericmustin]

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

[bengl]

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

[ericmustin]

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

[ericmustin]

Thanks for the feedback @bengl , makes sense to me, I've cleaned up the bits mentioned in review and added some additional bluebird tests here for the mechanics introduced for tracking library copies. The tests did require a bit of utility code to isolate testing the unpatch calls that probably ought to get abstracted into a generalised spec utility at some point, but didn't want the scope of this to get out of hand.

[bengl]

For testing purposes, I think we're less concerned with the implementation details (except that they're not hanging around after unpatch). The previously added test already ensures that it's been patched. The 'unpatching' tests ensure that we've properly unpatched, so we're good with that I think.

[ericmustin]

k makes sense, as long as the hidden property logic is specified in the unpatching section i think that's fine, updated this patching spec removed

[rochdev]

Left a few comments for the tests, but otherwise code LGTM.

[guidsen]

Any progress on this? Eagerly waiting for this to be implemented. Want to use Tracing in our production environment :D!

[rochdev]

@guidsen The plan is to include it in the next release. There are just a few small requested changes left and then it's good to go.

[guidsen]

@guidsen The plan is to include it in the next release. There are just a few small requested changes left and then it's good to go.

Great to hear. Thanks for the effort.

[ericmustin]

tried to address all the last round of feedback @rochdev , i think we're there, but let me know what you think.

[rochdev]

LGTM.

@bengl Please do a second pass since you had requested changes. It looks like everything has been addressed.

[guidsen]

Hey there. Nice that this is being merged. When are you planning to release this?

[rochdev]

@guidsen Released in 0.17.0. Sorry for the delay!

[guidsen]

No worries. Thanks for the help guys!