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(instrumentation-mongoose): Support v7 and v8 #2353

Merged
merged 5 commits into from
Aug 12, 2024

Conversation

onurtemizkan
Copy link
Contributor

Resolves: #1606

Which problem is this PR solving?

Related: getsentry/sentry-javascript#11499

  • This PR adds support for Mongoose versions 7 and 8 to the instrumentation.

Short description of the changes

Mongoose 7

  • The instrumentation already supports async Mongoose operations.
  • Moved tests using callbacks into a separate Mongoose v5/v6 test file
  • Skipped instrumentation for deprecated remove Mongoose Docs

Mongoose 8

  • Skipped instrumentation for deprecated count Mongoose Docs
  • Skipped instrumentation for deprecated findOneAndRemove Mongoose Docs

The current set of tests is passing without any issues, except findOneAndUpdate, which starts to create only one span, starting from version 7. I separated that test for v7/v8.

To avoid separating the package because of type mismatches, I have added a few // @ts-expect-errors inside tests with their reasons.

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.56%. Comparing base (dfb2dff) to head (a373fc8).
Report is 209 commits behind head on main.

Files Patch % Lines
...gins/node/instrumentation-mongoose/src/mongoose.ts 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2353      +/-   ##
==========================================
- Coverage   90.97%   90.56%   -0.42%     
==========================================
  Files         146      157      +11     
  Lines        7492     7631     +139     
  Branches     1502     1574      +72     
==========================================
+ Hits         6816     6911      +95     
- Misses        676      720      +44     
Files Coverage Δ
...gins/node/instrumentation-mongoose/src/mongoose.ts 98.43% <94.11%> (ø)

... and 74 files with indirect coverage changes

@onurtemizkan onurtemizkan force-pushed the mongoose-v7-v8 branch 7 times, most recently from a30b2d8 to 6075b12 Compare July 25, 2024 14:56
@trentm
Copy link
Contributor

trentm commented Jul 31, 2024

@onurtemizkan I haven't reviewed this PR, yet. A side issue: I notice that you are force-pushing. If you are able, please don't force-push to a PR branch, at least not after there have been any reviews on it. It makes it harder to do subsequent re-reviews because it isn't clear what has changed from a previous commit.

@onurtemizkan
Copy link
Contributor Author

Thanks for looking into this @trentm, and yes I'll be careful not to force-push on reviewed code. FWIW these last couple were rebases on main.

Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This looks good, modulo a couple suggestions below.

Please also apply this change to the README.md:

diff --git a/plugins/node/instrumentation-mongoose/README.md b/plugins/node/instrumentation-mongoose/README.md
index c5500b14..31a19b4e 100644
--- a/plugins/node/instrumentation-mongoose/README.md
+++ b/plugins/node/instrumentation-mongoose/README.md
@@ -17,7 +17,7 @@ npm install --save @opentelemetry/instrumentation-mongoose

 ## Supported Versions

-- [`mongoose`](https://www.npmjs.com/package/mongoose) versions `>=5.9.7 <7`
+- [`mongoose`](https://www.npmjs.com/package/mongoose) versions `>=5.9.7 <9`

 ## Usage

plugins/node/instrumentation-mongoose/.tav.yml Outdated Show resolved Hide resolved
@trentm trentm merged commit 770130a into open-telemetry:main Aug 12, 2024
21 checks passed
@trentm trentm mentioned this pull request Aug 13, 2024
trentm added a commit to renovate-bot/opentelemetry-js-contrib that referenced this pull request Aug 15, 2024
Note the warning about unhelpful npm behaviour in this workspace
when the version of mongodb to test (by TAV) matches the version
installed for the instrumentation-mongoose workspace. The version
in instr-mongoose recently updated (in open-telemetry#2353), so it needs to
update here as well.
mydea added a commit to getsentry/sentry-javascript that referenced this pull request Sep 5, 2024
…13587)

This bumps all our internal OTEL instrumentation to their latest
version.

Mainly, this fixes two things:

* Mongoose now supports v7 & v8
open-telemetry/opentelemetry-js-contrib#2353
* A variety of bug fixes, including a problem with http.get in ESM mode
open-telemetry/opentelemetry-js#4857 - See:
https://github.com/open-telemetry/opentelemetry-js/releases/tag/experimental%2Fv0.53.0

Related:

* open-telemetry/opentelemetry-js#4975 Issue
about relaxing deps in "core" instrumentation packages
* PR to bump deps in `@prisma/instrumentation`:
prisma/prisma#25160
* PR to bump deps in `opentelemetry-instrument-remix`:
justindsmith/opentelemetry-instrumentations-js#52
* PR to bump deps in `opentelemetry-instrumentation-fetch-node`:
gas-buddy/opentelemetry-instrumentation-fetch-node#17
(which will also be superseded by
#13485 eventually)

* Closes #11499
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.

Support for Mongoose 7
3 participants