search

open-telemetry / opentelemetry-js-contrib

OpenTelemetry instrumentation for JavaScript modules
https://opentelemetry.io
Apache License 2.0
638 stars 475 forks source link

Support for Mongoose 7 #1606

Open carlpett opened 11 months ago

carlpett commented 11 months ago

Currently, instrumentation-mongoose explicitly does not support Mongoose 7: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/82267ab02f8d9b27613b5926089c42d04d4a4a7e/plugins/node/instrumentation-mongoose/src/mongoose.ts#L78-L83

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

pichlermarc commented 11 months ago

@carlpett outside help is definitely useful :slightly_smiling_face:

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 npm install in plugins/node/instrumentation-mongoose shows the full error log). So opening a PR that updates it (and the tests) would be appreciated. However, there might be a few gotchas when doing that (test backward compatibility and such), so reaching out to the component owner (@blumamir) first is definitely advised :slightly_smiling_face:

carlpett commented 10 months ago

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?

pichlermarc commented 10 months ago

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:

blumamir commented 10 months ago

Thanks @carlpett

As marc mentioned, we usually prefer to introduce new supported major version as a PR here with tav and a human eye to brief the changes and verify what needs to be updated to maintain stability and functionality of the instrumentation.

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:

  1. update the relevant line in instrumentation.ts as you already pointed out.
  2. Add the new range to tav.yml and run it to verify it is green for all new and old versions
  3. update the README where supported versions is advertised.
  4. Apply any fixes and adaptations to instrumentation code to support new patched functions signatures or data types.

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?

Yes. It is generally always a good recommendation to use up-to-date OTEL packages with wider support and bug fixes. We can't tell if a future major version of instrumented packages will break users code. Actually, the instrumentation can also break on minor and patches, since we patch internal functions which are not committed to follow semver and can introduce refactors for which we are not compatible. For this, we run test-all-versions daily to catch these problems as early as possible.

carlpett commented 4 months ago

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.

odorT commented 3 months ago

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.

richardsimko commented 1 month ago

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.

kevinxu12 commented 1 month ago

Yeah following here!

Walledgarden commented 1 month ago

I'd also love to see support for the latest Mongoose versions.

However, for those who might wonder about a "work-around": Using @opentelemetry/instrumentation-mongodb works with Mongoose, as it depends on that package. I've tested it and queries are being traced. I'm aware that this is not a perfect solution, but at least something, until the support for the latest mongoose version is there as well.