feat(mongodb4): added mongodb4 instrumentation #794

osherv · 2021-12-22T13:26:10Z

MongoDB4 Instrumentation!

dyladan · 2021-12-22T13:26:50Z

Is there a reason this is a new instrumentation instead of updating the existing mongodb instrumentation?

osherv · 2021-12-22T13:29:09Z

@dyladan
Yes :) Here's the related Issue:
#577
'I think we should support both of them since v3 is still massively used'

dyladan · 2021-12-22T13:36:26Z

I was not suggesting to drop support for v3, but to add support for v4 to the existing instrumentation using a new InstrumentationNodeModuleDefinition.

osherv · 2021-12-22T15:46:49Z

@tydhot
I tried, but the new instrumentation won't be applyed in that case (in the ut):

Because i think that otel search in one mongodb only
Is there any option to fix that without create a new package?

dyladan · 2021-12-22T18:31:29Z

In most other instrumentations where we patch multiple major versions, we only use the types from the latest version and use any or unknown or hand-rolled types for the other versions.

osherv · 2021-12-22T18:35:47Z

@dyladan Thanks! ill try!

codecov · 2021-12-30T10:10:17Z

Codecov Report

Merging #794 (a0ab0a4) into main (7510757) will decrease coverage by 8.51%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #794      +/-   ##
==========================================
- Coverage   95.29%   86.78%   -8.52%     
==========================================
  Files          10       23      +13     
  Lines         701     1286     +585     
  Branches      142      260     +118     
==========================================
+ Hits          668     1116     +448     
- Misses         33      170     +137

Impacted Files	Coverage Δ
...instrumentation-nestjs-core/src/instrumentation.ts	`95.65% <0.00%> (ø)`
...-instrumentation-nestjs-core/src/enums/NestType.ts	`100.00% <0.00%> (ø)`
...opentelemetry-instrumentation-restify/src/types.ts	`100.00% <0.00%> (ø)`
...opentelemetry-instrumentation-restify/src/utils.ts	`100.00% <0.00%> (ø)`
...try-instrumentation-nestjs-core/src/enums/index.ts	`100.00% <0.00%> (ø)`
...opentelemetry-instrumentation-mongodb/src/types.ts	`100.00% <0.00%> (ø)`
...telemetry-instrumentation-restify/src/constants.ts	`100.00% <0.00%> (ø)`
...tapackages/auto-instrumentations-node/src/utils.ts	`96.77% <0.00%> (ø)`
...nstrumentation-generic-pool/src/instrumentation.ts	`40.00% <0.00%> (ø)`
...nstrumentation-restify/src/enums/AttributeNames.ts	`100.00% <0.00%> (ø)`
... and 3 more

osherv · 2022-01-02T12:34:18Z

@dyladan Hey :)
The issue is almost ready!
Two last fails:

mongo4 is not supported for node < 12
Code coverage is now lower because the mongo3 tests is only under test-all-versions and not under tests.

Any idea how to handle with these? Thanks alot

rauno56

Looks good! Thanks!

Please remove console.log(getTestSpans()); from mongodb-v3.test.ts as well.

Some of the small changes(few newlines, typecast to any) could also be taken to v3 test file, but that's not a deal breaker.

plugins/node/opentelemetry-instrumentation-mongodb/package.json

rauno56 · 2022-01-05T11:56:09Z

mongo4 is not supported for node < 12

@osherv, please ignore the package in the CI matrix via lerna-extra-args in unit-test.yml. The same way as @opentelemetry/instrumentation-pino has been disabled.

osherv · 2022-01-06T15:15:54Z

@rauno56 Fixed everything! Thanks a lot for the review :)

rauno56 · 2022-01-06T16:41:45Z

Nice! Even the smallest of my nits now addressed! 👍 I'd love to get another review before we merge.

plugins/node/opentelemetry-instrumentation-mongodb/src/types.ts

rauno56 · 2022-01-07T13:15:26Z

❗ This cannot yet be merged. For some reason, the tests stay pending forever. Works locally.

@osherv, the only thing that stands out from the setup is the --parallel option. Could you please try removing that?

blumamir · 2022-01-09T16:33:59Z

plugins/node/opentelemetry-instrumentation-mongodb/package.json

@@ -7,7 +7,8 @@
  "repository": "open-telemetry/opentelemetry-js-contrib",
  "scripts": {
    "docker:start": "docker run -e MONGODB_DB=opentelemetry-tests -e MONGODB_PORT=27017 -e MONGODB_HOST=localhost -p 27017:27017 --rm mongo",
-    "test": "nyc ts-mocha --parallel -p tsconfig.json --require '@opentelemetry/contrib-test-utils' 'test/**/*.test.ts'",
+    "test": "nyc ts-mocha -p tsconfig.json --require '@opentelemetry/contrib-test-utils' 'test/**/mongodb-v4.test.ts'",


this change means we are running PullRequests CI tests only for v4, and stop running v3 tests on PRs.

opentelemetry-js-contrib/package.json

Line 23 in 81b3190

"test:ci:changed": "lerna run test --since origin/main",

something we might want to consider...

That is correct. I can't think about any other option. Suggestions?

Not a particularly good one
We can run tav here as well, possibly with just 2 versions. But that sounds clumsy and complex.
Let's wait for more minds on this one @rauno56 @dyladan

btw, The tests are only can run locally (we need mongodb server). So anyway i dont think i would change much

Yes, I usually spin up a mongo server locally with docker and run the tests against it.
CI tests also start a mongo server here, so the tests can pass.

We talked about this in the SIG. We recommend to test the most popular major version (in this case v3) and test other versions with TAV.

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts

linux-foundation-easycla · 2022-01-10T09:09:57Z

✅ Osher Vaknin (7a2734b, f171e3d, 177338f, b1d7aad, 652ae27, a65c4b3)

❌ The commit (af49bac ,6de0a7386a82b7e031aac51ebf99a718cc39612c ,28dd80f2744ce3820b603f99d86a362c7e1c4bfe ,af247e48e495f42c27f4368b4394dc05ec1e4f21 ,87efbc6a702758dffcc9f27d3908f82bf5e763e6 ,af3f981f827a71baa2a9ca5618cbad5834c020ce ,726294451bb304fcf385e5dbc246ac28acff1aa1 ,8333e5449ce915af798a77229f7e92edc725a65e ,33a729477f99aeccdc7c50fa8a5ef0dec521a11a ,d4ea2c6e947afb9a417e20ec0cbec279d8f45bef ,009f16f0bd65699ebb8db7e98756c24f32ad7342) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

blumamir · 2022-01-10T09:23:04Z

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts

+    let commandObj: Record<string, unknown>;
+    if (command?.documents) {
+      commandObj = command.documents[0];
+    } else if (command.cursors) {


command?.cursor

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts

rauno56 · 2022-01-10T11:19:27Z

I turned on streaming Lerna output to see that MongoDB tests fail, but do not terminate the process. That's why the tests never finish:
~~https://github.com/Rauno56/opentelemetry-js-contrib/runs/4760394725?check_suite_focus=true~~ GHA links are useless. The error was:

      Uncaught AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '127.0.0.1'
- 'localhost'
      + expected - actual

      -127.0.0.1
      +localhost
      
      at Object.assertSpans (test/utils.ts:88:10)
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb-v4.test.ts:380:11
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/utils.ts:622:5
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/operations/execute_operation.ts:111:45
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/utils.ts:622:5
      at completeEndSession (node_modules/mongodb/src/sessions.ts:281:9)
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/sessions.ts:293:7
      at maybePromise (node_modules/mongodb/src/utils.ts:607:3)
      at ClientSession.endSession (node_modules/mongodb/src/sessions.ts:263:24)
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/operations/execute_operation.ts:111:26
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/operations/insert.ts:126:7
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/operations/bulk_write.ts:58:7
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/utils.ts:264:17
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/utils.ts:255:9
      at executeCommands (node_modules/mongodb/src/bulk/common.ts:591:12)
      at resultHandler (node_modules/mongodb/src/bulk/common.ts:618:5)
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/utils.ts:622:5
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/operations/execute_operation.ts:114:9
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/node_modules/mongodb/src/cmap/connection_pool.ts:469:13
      at handleOperationResult (node_modules/mongodb/src/sdam/server.ts:608:5)
      at /.../opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts:122:47
      at AsyncLocalStorage.run (node:async_hooks:320:14)
      at AsyncLocalStorageContextManager.with (node_modules/@opentelemetry/context-async-hooks/src/AsyncLocalStorageContextManager.ts:40:36)
      at ContextAPI.with (node_modules/@opentelemetry/api/src/api/context.ts:77:42)
      at patchedEnd (src/instrumentation.ts:122:47)
      at MessageStream.messageHandler (node_modules/mongodb/src/cmap/connection.ts:753:5)
      at MessageStream.emit (node:events:390:28)
      at processIncomingData (node_modules/mongodb/src/cmap/message_stream.ts:167:12)
      at MessageStream._write (node_modules/mongodb/src/cmap/message_stream.ts:64:5)
      at writeOrBuffer (node:internal/streams/writable:389:12)
      at _write (node:internal/streams/writable:330:10)
      at MessageStream.Writable.write (node:internal/streams/writable:334:10)
      at Socket.ondata (node:internal/streams/readable:754:22)
      at Socket.emit (node:events:390:28)
      at addChunk (node:internal/streams/readable:315:12)
      at readableAddChunk (node:internal/streams/readable:289:9)
      at Socket.Readable.push (node:internal/streams/readable:228:10)
      at TCP.onStreamRead (node:internal/stream_base_commons:199:23)
      at TCP.callbackTrampoline (node:internal/async_hooks:130:17)

rauno56 · 2022-01-10T12:06:28Z

Regarding ☝️ That definitely needs to be fixed in two aspects:

errors from the tests should always terminate the process. Some combination of MongoDB and mocha doesn't currently work well, because the test is marked failed by mocha internally but it doesn't really throw and terminate. I tried it with the instrumentation disabled(but the rest of the setup there) - same thing. To reproduce just throw in the callback of the mongo command. The only thing that comes to mind right now is to catch errors in callbacks and call done(err); ourselves. 😞
The thrown error comes from the fact that mongo resolves localhost and stores the IP on the field we check against localhost. To bypass that, we should set the host to 127.0.0.1 from the get-go to avoid the discrepancy. That can be done

zombispormedio · 2022-01-25T10:38:19Z

any update about this PR? I am interested in

osherv · 2022-01-25T12:05:13Z

any update about this PR? I am interested in

Sry for the delay, i had to omicron virus -_-..
I'm working on it right now

zombispormedio · 2022-01-25T13:45:01Z

Sry for the delay, i had to omicron virus -_-.. I'm working on it right now

Sorry, just wondering. I hope you had a good recovery. Thank you.

linux-foundation-easycla · 2022-01-25T16:12:15Z

✅ Osher Vaknin (7a2734b, f171e3d, 177338f, b1d7aad, 652ae27, a65c4b3, 6dbc6d5, ca4d020, fe9bb27, 196d77e)

❌ The commit (af49bac ,6de0a7386a82b7e031aac51ebf99a718cc39612c ,28dd80f2744ce3820b603f99d86a362c7e1c4bfe ,af247e48e495f42c27f4368b4394dc05ec1e4f21 ,87efbc6a702758dffcc9f27d3908f82bf5e763e6 ,af3f981f827a71baa2a9ca5618cbad5834c020ce ,726294451bb304fcf385e5dbc246ac28acff1aa1 ,8333e5449ce915af798a77229f7e92edc725a65e ,33a729477f99aeccdc7c50fa8a5ef0dec521a11a ,d4ea2c6e947afb9a417e20ec0cbec279d8f45bef ,009f16f0bd65699ebb8db7e98756c24f32ad7342) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

linux-foundation-easycla · 2022-01-25T16:14:08Z

✅ Osher Vaknin (7a2734b, f171e3d, 177338f, b1d7aad, 652ae27, a65c4b3, 6dbc6d5, ca4d020, fe9bb27, 196d77e, 4fa8252)

❌ The commit (af49bac ,6de0a7386a82b7e031aac51ebf99a718cc39612c ,28dd80f2744ce3820b603f99d86a362c7e1c4bfe ,af247e48e495f42c27f4368b4394dc05ec1e4f21 ,87efbc6a702758dffcc9f27d3908f82bf5e763e6 ,af3f981f827a71baa2a9ca5618cbad5834c020ce ,726294451bb304fcf385e5dbc246ac28acff1aa1 ,8333e5449ce915af798a77229f7e92edc725a65e ,33a729477f99aeccdc7c50fa8a5ef0dec521a11a ,d4ea2c6e947afb9a417e20ec0cbec279d8f45bef ,009f16f0bd65699ebb8db7e98756c24f32ad7342) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

osherv · 2022-01-25T16:15:45Z

@rauno56 @blumamir
Hey! Good to be back after the Omicron XD.
Fix all of your comments. I need to fix my CLA, so the moment you approves my PR, ill open new branch with these changes.
:)

blumamir · 2022-01-25T16:25:25Z

@rauno56 @blumamir Hey! Good to be back after the Omicron XD. Fix all of your comments. I need to fix my CLA, so the moment you approves my PR, ill open new branch with these changes. :)

Good to have you back and thanks for the great work. Hope you are feeling well now :)
You can also open a new PR with the fixed CLA and I'll finish the review there. It will probably take me a day or two to get to it.

.github/workflows/unit-test.yml

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts

linux-foundation-easycla · 2022-02-03T15:10:04Z

✅ Osher Vaknin (7a2734b, f171e3d, 177338f, b1d7aad, 652ae27, a65c4b3, 6dbc6d5, ca4d020, fe9bb27, 196d77e, 4fa8252, a0ab0a4)

❌ The commit (af49bac ,6de0a7386a82b7e031aac51ebf99a718cc39612c ,28dd80f2744ce3820b603f99d86a362c7e1c4bfe ,af247e48e495f42c27f4368b4394dc05ec1e4f21 ,87efbc6a702758dffcc9f27d3908f82bf5e763e6 ,af3f981f827a71baa2a9ca5618cbad5834c020ce ,726294451bb304fcf385e5dbc246ac28acff1aa1 ,8333e5449ce915af798a77229f7e92edc725a65e ,33a729477f99aeccdc7c50fa8a5ef0dec521a11a ,d4ea2c6e947afb9a417e20ec0cbec279d8f45bef ,009f16f0bd65699ebb8db7e98756c24f32ad7342) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

blumamir · 2022-02-06T09:08:18Z

Thanks for addressing all the comments and for adding the support for v4 🥇
After the CLA is fixed, this can be merged as far as I am concerned

osherv · 2022-02-06T09:22:41Z

Thanks! next pr will be muck more quicker :)
I'll open new pr with the fixed CLA

blumamir · 2022-02-06T12:26:56Z

closing as this PR migrated to #869 due to CLA issues

feat(mongodb4): added mongodb4 instrumentation

af49bac

osherv requested a review from a team December 22, 2021 13:26

feat(mongodb4): merged mongodb4 instrumentation to mongodb plugin

6de0a73

Osher Vaknin and others added 2 commits December 30, 2021 13:56

feat(mongodb4): fixed ut

28dd80f

Merge branch 'main' into feat/add-mongodb4-instrumentation

7a2734b

vmarchaud added the enhancement New feature or request label Dec 30, 2021

Osher Vaknin and others added 2 commits January 2, 2022 11:54

feat(mongodb4): fixed ut connection timeout

af247e4

Merge branch 'main' into feat/add-mongodb4-instrumentation

f171e3d

rauno56 approved these changes Jan 5, 2022

View reviewed changes

plugins/node/opentelemetry-instrumentation-mongodb/package.json Outdated Show resolved Hide resolved

Osher Vaknin added 4 commits January 6, 2022 16:39

feat(mongodb): fixed most of Rauno's CR

87efbc6

feat(mongodb): fixed most of Rauno's CR

af3f981

feat(mongodb): fixed Rauno's CR

7262944

feat(mongodb): updated nyrc exclude

8333e54

osherv and others added 2 commits January 6, 2022 17:16

Merge branch 'main' into feat/add-mongodb4-instrumentation

177338f

feat(mongodb): updated README

33a7294

dyladan reviewed Jan 6, 2022

View reviewed changes

plugins/node/opentelemetry-instrumentation-mongodb/src/types.ts Outdated Show resolved Hide resolved

feat(mongodb): fixed Daniel's CR

d4ea2c6

dyladan approved these changes Jan 6, 2022

View reviewed changes

blumamir reviewed Jan 9, 2022

View reviewed changes

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts Outdated Show resolved Hide resolved

blumamir reviewed Jan 9, 2022

View reviewed changes

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts Outdated Show resolved Hide resolved

feat(mongodb4): fixed Amir's CR

a65c4b3

blumamir reviewed Jan 10, 2022

View reviewed changes

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts Show resolved Hide resolved

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts Show resolved Hide resolved

feat(mongo): default mongo host is now the IP

6dbc6d5

osherv added 3 commits January 25, 2022 17:10

feat(mongo): mongodb4 tests are now using always promises

ca4d020

feat(mongo): mongodb3 tests are now using always promises

fe9bb27

feat(mongo): fixed Amir's CR

196d77e

Merge branch 'main' into feat/add-mongodb4-instrumentation

4fa8252

blumamir reviewed Jan 26, 2022

View reviewed changes

.github/workflows/unit-test.yml Outdated Show resolved Hide resolved

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts Outdated Show resolved Hide resolved

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts Outdated Show resolved Hide resolved

feat(mongo): fixed Amir's CR

a0ab0a4

blumamir approved these changes Feb 6, 2022

View reviewed changes

osherv mentioned this pull request Feb 6, 2022

feat(mongodb4): added mongodb4 instrumentation #869

Merged

blumamir closed this Feb 6, 2022

feat(mongodb4): added mongodb4 instrumentation #794

feat(mongodb4): added mongodb4 instrumentation #794

Conversation

osherv commented Dec 22, 2021

dyladan commented Dec 22, 2021

osherv commented Dec 22, 2021

dyladan commented Dec 22, 2021

osherv commented Dec 22, 2021 • edited Loading

dyladan commented Dec 22, 2021

osherv commented Dec 22, 2021

codecov bot commented Dec 30, 2021 • edited Loading

Codecov Report

osherv commented Jan 2, 2022

rauno56 left a comment

Choose a reason for hiding this comment

rauno56 commented Jan 5, 2022 • edited Loading

osherv commented Jan 6, 2022

rauno56 commented Jan 6, 2022

rauno56 commented Jan 7, 2022 • edited Loading

blumamir Jan 9, 2022 • edited Loading

Choose a reason for hiding this comment

osherv Jan 9, 2022

Choose a reason for hiding this comment

blumamir Jan 9, 2022

Choose a reason for hiding this comment

osherv Jan 9, 2022

Choose a reason for hiding this comment

blumamir Jan 9, 2022

Choose a reason for hiding this comment

dyladan Jan 12, 2022

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jan 10, 2022

blumamir Jan 10, 2022

Choose a reason for hiding this comment

osherv Jan 24, 2022

Choose a reason for hiding this comment

rauno56 commented Jan 10, 2022 • edited Loading

rauno56 commented Jan 10, 2022

zombispormedio commented Jan 25, 2022

osherv commented Jan 25, 2022

zombispormedio commented Jan 25, 2022

linux-foundation-easycla bot commented Jan 25, 2022

linux-foundation-easycla bot commented Jan 25, 2022

osherv commented Jan 25, 2022 • edited Loading

blumamir commented Jan 25, 2022

linux-foundation-easycla bot commented Feb 3, 2022

blumamir commented Feb 6, 2022

osherv commented Feb 6, 2022

blumamir commented Feb 6, 2022

osherv commented Dec 22, 2021 •

edited

Loading

codecov bot commented Dec 30, 2021 •

edited

Loading

rauno56 commented Jan 5, 2022 •

edited

Loading

rauno56 commented Jan 7, 2022 •

edited

Loading

blumamir Jan 9, 2022 •

edited

Loading

rauno56 commented Jan 10, 2022 •

edited

Loading

osherv commented Jan 25, 2022 •

edited

Loading