fix(mongodb): mongodb v6.4.0 & 6.5.0 support

[david-luna]

Which problem is this PR solving?

the new version of mongodb v6.4.0 comes with some internal changes that break the instrumentation. Drtails of the changes are described in #1983 (comment)

Closes: #1983

Short description of the changes

patches of the changed methods have been refactored to accept both implementations

• handling callbacks for <6.4.0
• handling returned promises for >=6.4.0

[trentm]

It feels slightly messy to be passing in this patchedCallback that isn't going to be used.
Thoughts on a separate patch function for mongodb >=6.4.0 -- this one would just do the promise thing.

[david-luna]

my intention was to not split the patch logic of this function to 2 separate patches. I do agree is not optimal to create a function that may not be used. I'll try to come with a different approach

[trentm]

If this is done, then the default "test" script will need to change above, right?

[david-luna]

you're right

[trentm]

Similar Q to above: would separate patch functions (one that uses the callback, one that uses the promise) be clearer? or would they just be a pain?

[david-luna]

here the patchedCallback can be reused because basically the API change has been to "promisify" the function. A separate patch is not complicated, it's almost an identical copy.

[codecov]

Codecov Report

Merging #2001 (4c1aac9) into main (dfb2dff) will decrease coverage by 0.34%.
Report is 18 commits behind head on main.
The diff coverage is 71.11%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2001      +/-   ##
==========================================
- Coverage   90.97%   90.63%   -0.34%     
==========================================
  Files         146      146              
  Lines        7492     7488       -4     
  Branches     1502     1494       -8     
==========================================
- Hits         6816     6787      -29     
- Misses        676      701      +25     

Files	Coverage Δ
...etry-instrumentation-mongodb/src/internal-types.ts	100.00% <ø> (ø)
...try-instrumentation-mongodb/src/instrumentation.ts	47.42% <71.11%> (-5.85%)	⬇️

... and 7 files with indirect coverage changes

[david-luna]

@trentm I did the split you suggested but partially. I think it was overkill to split the logic of the connect patch.

Now I'm hitting a problem in test-all-versions script. It is failing for [email protected] in the beforeEach method of the tests which is quite odd.

if I log the version under test...

--- a/plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb-v4.test.ts
+++ b/plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb-v4.test.ts
@@ -56,6 +56,9 @@ describe('MongoDBInstrumentation-Tracing-v4', () => {
   const COLLECTION_NAME = 'test-traces';
   const URL = `mongodb://${HOST}:${PORT}/${DB_NAME}`;

+  const version = require('mongodb/package.json').version;
+  console.log(`testing mongodb@${version}`);
+
   let client: mongodb.MongoClient;
   let collection: mongodb.Collection;

it turns out we get a different version for testing

> @opentelemetry/[email protected] test-v4
> nyc ts-mocha -p tsconfig.json --require '@opentelemetry/contrib-test-utils' 'test/mongodb-v4-v5-v6.metrics.test.ts' 'test/**/mongodb-v4.test.ts'

testing [email protected]

  MongoDBInstrumentation-Metrics
    ✓ Should add connection usage metrics (101ms)
    ✓ Should add disconnection usage metrics (213ms)

  MongoDBInstrumentation-Tracing-v4
    Instrumenting query operations
      1) "before each" hook: mongoBeforeEach for "should create a child span for insert"
      2) "after each" hook for "should create a child span for insert"


  2 passing (4s)
  2 failing

....

@trentm I think you got into a similar situation but I can't find the issue or PR. Do you recall having a similar issue? 🤔

[trentm]

@trentm I think you got into a similar situation but I can't find the issue or PR. Do you recall having a similar issue? 🤔

Yup, it was described here: #1938 (comment)
We get this behaviour because of a pinned "mongodb" transitive dep from instrumentation-mongoose:

[~/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-mongodb (git:1983-mongodb-6-4-support)]
% npm ls mongodb
[email protected] /Users/trentm/tm/opentelemetry-js-contrib2
└─┬ @opentelemetry/[email protected] -> ./plugins/node/opentelemetry-instrumentation-mongodb
  └── [email protected]

[~/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-mongodb (git:1983-mongodb-6-4-support)]
% npm install --no-save [email protected]
...

[~/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-mongodb (git:1983-mongodb-6-4-support)]
% npm ls mongodb
[email protected] /Users/trentm/tm/opentelemetry-js-contrib2
└─┬ @opentelemetry/[email protected] -> ./plugins/node/opentelemetry-instrumentation-mongodb
  └── [email protected]

The looking for any npm install felt it didn't need to install v4.17.1 locally:

[~/tm/opentelemetry-js-contrib2 (git:1983-mongodb-6-4-support)]
% npm ls mongodb
[email protected] /Users/trentm/tm/opentelemetry-js-contrib2
├─┬ @opentelemetry/[email protected] -> ./plugins/node/opentelemetry-instrumentation-mongodb
│ └── [email protected]
└─┬ @opentelemetry/[email protected] -> ./plugins/node/instrumentation-mongoose
  └─┬ [email protected]
    └── [email protected]

Yuck. This has to be an npm bug. I'm not sure how to cope with this in general.
We discussed (in DMs) explicitly skipping v4.17.1 in the .tav.yml file. That's a hack, but I can't think of anything better.

[trentm]

Thanks for humouring me on the splitting the callback vs. promise patching. That looks like it a was a fair amount of extra work.

LGTM, though I think the .tav.yml update should be added before merge.

[trentm]

Note to reviewers: This was discussion in the OTel JS SIG this week as a fix for the code-coverage check failures. Testing coverage only with node v14 artificially results in coverage looking poor for a unit-test run that skips all or most tests for the (now old) node v14.

[pichlermarc]

Thanks for taking care of this 🙂

[david-luna]

So the split added new code that isn't covered by the tests since is meant to run in earlier versions. I'll see what I can do about it

[david-luna]

By looking at the docs there is no way to tell codecov that a specific code path is expected to not to be covered. I guess the solution would be to make use of merge reports but IMO this is out of scope of this PR (the impact is probably high)

@pichlermarc do you think is okay to move forward and have a follow-up PR about merging coverage reports from unit testing?

[trentm]

I agree dealing with merging of coverage reports is out of scope for this PR.