-
Notifications
You must be signed in to change notification settings - Fork 495
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
Support for Mongoose 7 #1606
Comments
@carlpett outside help is definitely useful 🙂 That there are no spans on mongoose >=7 is completely intentional. When a new major version of a library is published, changes may be breaking. To prevent breaking user's code, the instrumentation does not automatically register until the version is manually updated by the component owner (or another contributor) after verifying that everything still works. The problem in this specific case seems to be just that unit tests need to be updated, as they don't compile anymore with mongoose 7 (lerna in the CI hides some info of what's going wrong, so just running |
So, finally had some time to take a look at this. As mentioned, the happy path works for me, in a code base which has fixed the breaking changes between v6 and v7. However, just bumping the version in this library does not work quite as smoothly... The main issue is the dropped callback support. This appears to require a somewhat large refactoring to remove it from this library. Also, what are the compatibility concerns? Do you typically say that it is required to bump instrumented libraries in your application when updating this library? |
We usually don't require that our users update their library to continue receiving telemetry, though this is up to the component owner to decide.It's hard to say for me without knowing the exact details of the instrumented library, changes and instrumentation library, but there are usually two ways to handle this:
|
Thanks @carlpett As marc mentioned, we usually prefer to introduce new supported major version as a PR here with I could find some time to look at it soon, but your help will be highly appreciated if you can open a PR to do the work + describe what changed so it's easy to review. Please feel free to reach out in slack or here if you need to discuss changes or assistance in applying a fix. note that when adding support to a new version, you need to:
Yes. |
I gave up last year because I ran out of time, mostly forgot about it, and then now had to rediscover everything again :) Looking at it, if the instrumentation should support both <7 and >=7, it might be quite complicated. The removal of callback support changes a lot, which seems quite entangled with being supported throughout the library. Would it be feasible to have a different implementation for the older version? Also, I'm unable to get the tests running, I'll open a separate ticket for that to avoid side-tracking here. |
Hi, I have just started testing out OpenTelemetry in our test env and found it super useful. Most of libs are working out of the box with auto injection library (thank you a lot!), except mongoose. We use ^7.0.4 version of mongoose, seems it is not supported yet. What are the plans? I am not JS developer, otherwise would be involved with development part of it, too. |
Since this issue was opened mongoose has released another major version. Any plans on supporting those? Mongoose 7.0 has been out for over a year now. |
Yeah following here! |
I'd also love to see support for the latest Mongoose versions. However, for those who might wonder about a "work-around": Using |
Currently, instrumentation-mongoose explicitly does not support Mongoose 7:
opentelemetry-js-contrib/plugins/node/instrumentation-mongoose/src/mongoose.ts
Lines 78 to 83 in 82267ab
Testing on one of my recently upgraded services that are now span-less, things appear to work out of the box if I just change that constraint. However, I suspect there might be edge cases or breakages I'm not triggering.
Renovate seems to have given up on the automatic bump here, #1420, but the
lerna
error is pretty opaque to me. Is outside help useful here, or mainly a question of a maintainer having time to look at it?cc @blumamir since I saw you in the component_owners file
The text was updated successfully, but these errors were encountered: