fix(instrumentation-mongoose): Fix instrumentation for Mongoose Model methods, save() and remove(), by passing options argument to original method calls

[ferrelucas]

Which problem is this PR solving?

Fixes #1678

This PR fixes the issue where transactions cannot be used for the Model methods save() and remove() (and their aliases eg. create()) because after instrumentation is applied to these methods, specifically when both options and callback are defined, the final method call loses the first argument option making the final call not take the option.session into consideration when making database operations.
This could lead to save updates in transactions not being properly rolled back as well as multiple operations that are supposed to share the same session not being able to do so because their calls would eventually be made without the session reference.

Short description of the changes

• Pass the original args into handleCallbackResponse()
• Updates the implementation of handleCallbackResponse() so that we create newArgs including the original options and the instrumentedcallback function.
• Added tests to cover the base case of empty-object but defined options argument with callback
• Added tests cases to cover committed and aborted transactions

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: ferrelucas / name: Lucas Ferreira (908b8af, b414324, 763a1a2, adde5d2, 4d99a63, 019d49d, d3294fe, 1381823, 0a2ffb3, 95c1042, d609e1f, 6b7cf21, d89bc0f, f205174, 8e9630f, b6b04f7, cbce832, 7d3b460, fac1251, 8aa008e, 2283303, a8219a1, ea60ff1, 784e99e, 57a9e9f, 4a6a12b, a4336d3, 453f3a7, 1361a24)
• ✅ login: blumamir / name: Amir Blum (1764acf, 7f12860)

[ferrelucas]

ℹ️ Note this replacer function was only introduced here to avoid an error due to the Session object having cyclical reference

[ferrelucas]

Hi @blumamir, wondering if you could have a look at this PR when you have some time?

[ferrelucas]

@open-telemetry/javascript-approvers Hi there, wondering if somebody could have a look at this PR?

[ferrelucas]

Hello, @blumamir and @open-telemetry/javascript-approvers just reaching out to check if anyone could have a look at this PR?

[blumamir]

Sorry for the time it took to review and thank you for creating a fix for this issue 🙏

Not sure I fully understood the logic in this PR regarding adding new callback to the existing array.

[ferrelucas]

@blumamir , I've update the PR to simplify the usage of args

Please let me know what you think
Thanks!

[blumamir]

LGTM

Thanks again for the fix and sorry it took so long to review 🙏

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.01%. Comparing base (dfb2dff) to head (a4336d3).
Report is 139 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2009      +/-   ##
==========================================
- Coverage   90.97%   90.01%   -0.97%     
==========================================
  Files         146      140       -6     
  Lines        7492     6909     -583     
  Branches     1502     1457      -45     
==========================================
- Hits         6816     6219     -597     
- Misses        676      690      +14     

see 50 files with indirect coverage changes

[ferrelucas]

No worries!
Thanks for the review! 🙌

[blumamir]

@ferrelucas let me know if you need any help fixing the CI issues. once it is green we can merge and release this fix

[ferrelucas]

@blumamir thanks, I have fixed the lint issues.

I just realized the Unit Test issues are giving the MongoServerError: Transaction numbers are only allowed on a replica set member or mongos error message.

I happen to not have this issue running the tests locally because I was already running a MongDB Replica Set.
It seems we would need to update the MongoDB test setup to use replica sets so it can support transactions.

I can have a better look on how to do it soon - any directions on it would be helpful
Thanks!

[ferrelucas]

Hi @blumamir I've taken one of the CI unit tests as example Run test-all-versions (20).
I've seen that the MongoDB setup is in the step called Initialize containers, under Starting mongo service container.

It is running:

/usr/bin/docker create --name fa2fac6564d8478ebd791968ddae63b9_mongo_11c61e --label 62ce9a --network github_network_60aacba5f9dc4f70be079739b8ce42a8 --network-alias mongo -p 27017:27017  -e GITHUB_ACTIONS=true -e CI=true mongo

I think we could add the --replSet parameter to the command to start the MongoDB instance, then initialize the replica set.

I believe I don't have permissions to make these changes in the Github Actions
Would it be feasible to do it on your side?
Thanks!

[blumamir]

@blumamir thanks, I have fixed the lint issues.

I just realized the Unit Test issues are giving the MongoServerError: Transaction numbers are only allowed on a replica set member or mongos error message.

I happen to not have this issue running the tests locally because I was already running a MongDB Replica Set. It seems we would need to update the MongoDB test setup to use replica sets so it can support transactions.

I can have a better look on how to do it soon - any directions on it would be helpful Thanks!

perhaps we can use another option property instead of session to make sure it is passed correctly to the driver?

I can see you use this code to test passing both options and callback

await user.save({ session }, async () => {

we can consider using something else instead of session which is simpler and does not require replica set https://mongoosejs.com/docs/6.x/docs/api/model.html#model_Model-save

[blumamir]

I believe I don't have permissions to make these changes in the Github Actions
Would it be feasible to do it on your side?

If I am not mistake, the mongo service is started here which you can edit, but I really think it would be easier to just use some other option for the test instead of session, if one of them is simpler.

[ferrelucas]

@blumamir thanks! I've updated the PR unit tests to use wtimeout instead of session in the options object

[ferrelucas]

Hey, @blumamir thanks again for the review! 🙌