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

feat(mongodb): support v5 #1451

Merged

Conversation

GuillaumeDeconinck
Copy link
Contributor

@GuillaumeDeconinck GuillaumeDeconinck commented Apr 2, 2023

Which problem is this PR solving?

  • @opentelemetry/instrumentation-mongodb: Handle version 5 of MongoDB NodeJS Driver

Short description of the changes

  • feat(mongodb): Extend range of versions allowed for v4 patches to include v5
  • test(mongodb) Add test cases for v5
    • Duplicate of v4 with just callbacks removed (as it has been removed in v5)
  • chore(mongodb): Add command and configuration for v5 tests (package.json and .tav.yml)
  • chore(mongodb): Renamed package.json command test-new-versions to test-v4 (and adapted .tav.yml for it)

Notes

  • Unit tests of v4 are apparently running fine on MongoDB v5 when callbacks are adapted to Promises
  • Tested quickly on an app with jaeger, otel collector and signoz, it seems to work fine too
  • I'm not sure if there's any breaking change impacting the tracing in the V5. From what I understand, it's not the case

Feel free to comment/adapt/suggest things. I'm still learning on this kind of stuff 😉

@GuillaumeDeconinck GuillaumeDeconinck requested a review from a team April 2, 2023 11:13
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 2, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot requested a review from osherv April 2, 2023 11:14
@GuillaumeDeconinck
Copy link
Contributor Author

Actually we tested a bit more today and noticed that the details of the queries are not present (e.g. the find parameters). I'll try to look at it soon.

@GuillaumeDeconinck GuillaumeDeconinck marked this pull request as draft April 3, 2023 13:02
@Eji4h
Copy link

Eji4h commented May 5, 2023

I'm waiting for it.

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #1451 (df6c722) into main (8499b16) will decrease coverage by 0.82%.
The diff coverage is 88.88%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1451      +/-   ##
==========================================
- Coverage   96.13%   95.32%   -0.82%     
==========================================
  Files          14       16       +2     
  Lines         906     1005      +99     
  Branches      197      208      +11     
==========================================
+ Hits          871      958      +87     
- Misses         35       47      +12     
Impacted Files Coverage Δ
...strumentation-document-load/src/instrumentation.ts 98.13% <88.88%> (-0.86%) ⬇️

... and 2 files with indirect coverage changes

@GuillaumeDeconinck GuillaumeDeconinck force-pushed the feat/mongodb-support-v5 branch from 34e8bc0 to e9118d3 Compare May 11, 2023 12:09
@GuillaumeDeconinck
Copy link
Contributor Author

For what I said before:

Actually we tested a bit more today and noticed that the details of the queries are not present (e.g. the find parameters). I'll try to look at it soon.

Nevermind, I thought it automatically included all the parameters of find(), but the enhancedDatabaseReporting option is actually used for that. By default, only the first level is included, like before.

I think the PR can be reviewed.

@GuillaumeDeconinck GuillaumeDeconinck marked this pull request as ready for review May 11, 2023 12:15
@karanssj4
Copy link

@osherv any update on this? v5 is working similar to v4 except for dropping callback support

@icco
Copy link

icco commented May 27, 2023

I'm not an approver, but this looks good to me (there are currently merge conflicts though). We'd love to see this merged.

@GuillaumeDeconinck
Copy link
Contributor Author

I quickly checked but it seems there are some tests failing now after fixing the conflicts.
I'll likely be able to fix these in the coming days.
Once done, I'll ping another maintainer as the (randomly?) assigned one doesn't seem to be active for the moment.

@GuillaumeDeconinck
Copy link
Contributor Author

GuillaumeDeconinck commented May 31, 2023

I've adapted the code based on the changes on main. The unit tests are all passing locally (with tav).

I had to use the same database name for all the tests, as right now the instrumentation only allows one poolName (and not one per connection). That's likely something to fix in another issue/PR.

@haddasbronfman Hello, sorry to ping you like that. I see you checked/worked on another PR for MongoDB. Would it possible to review this one when you have some time ? Thanks !

@icco
Copy link

icco commented Jun 2, 2023

@legendecas or @dyladan could we get a review here?

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Just a few questions from my side. 🙂

@haddasbronfman I know you've been contributing to this instrumentation lately, would you mind taking a look? 🙂

@GuillaumeDeconinck
Copy link
Contributor Author

@pichlermarc Thanks for the review and for maintaining the branch up-to-date with master 🙂
I've updated the PR based on your comments

Copy link
Member

@haddasbronfman haddasbronfman left a comment

Choose a reason for hiding this comment

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

LGTM. thank you very much for handling this !!
 only one comment from my side.

@icco
Copy link

icco commented Jun 15, 2023

Hi, any chance we could get this reviewed? My company's traces are broken until we get this released

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks great, thanks 🙂
Waiting for one more CI run before merging 🙂

@pichlermarc
Copy link
Member

Looks like there's a problem with our workflows - we'll have to take care of #1541 first to make CI pass. Sorry for the delay. 😞

@haddasbronfman haddasbronfman merged commit 05c4e9e into open-telemetry:main Jul 4, 2023
@dyladan dyladan mentioned this pull request Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants