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

chore: fix contrib build #1374

Merged
merged 7 commits into from
Feb 7, 2023
Merged

Conversation

kobi-co
Copy link
Contributor

@kobi-co kobi-co commented Feb 6, 2023

contrib CI and build is broken for several days due to 2 dependencies that were released lately and contained breaking changes:

MongoDB

MongoDB instrumentation supports both v3 and v4. for v3 we depend on the @types/mongodb@3 package, which in turn depends on @types/bson@* (This is a bug in definitely typed!). MongoDB released v5 last week which contains breaking changes (removing the ObjectID export). Since the types dependency does not limit a version range, the new version is used and since it's not compatible with v3 typescript fails.

See related issue in defiantly typed.

To fix this, we have to depend on the real version of bson and not the incompatible latest.

Things we tried and didn't work:

  • remove instrumentation support for mongodb@3 - it has 1M downloads in npm, more than v4
  • fix the wrong dependency range in definitely typed - since v4 is distributed with types, the package is deprecated and removed from the code base. This fix will need to be applied to v3 of @types/mongodb and we couldn't find any process for updating and releasing types packages for older major versions.
  • use npm "overrides" - introduced in npm@7 and we support node14 which uses npm@6. Thus not feasible at the moment.
  • copy the types declarations into the package and drop the dependency on the buggy @types/mongodb altogether - this failed to compile and we were not knowledgable enough in ts to figure out how to make it work.

The fix that resolved the issue was to introduce our own dependency on @types/bson which links to the correct version.

Winston

Winston depends on logform@^2.4.0 which published a new version yesterday v2.5.0.

This version introduced types in the public API that are not a dependency of the package, thus breaking everyone using typescript. See more discussion in this issue and a fix (not yet merged) in this PR. Since the PR is not yet merged and the fix is not published, we need to fix this in our codebase to make our build work. In the future, when winston fixes the issue, we can remove the dependency on @types/triple-beam.

The other issue is that the new logform version introduced this change which is only implemented in typescript in this PR which is not part of our current dependency 4.3.5. To fix this, we had to bump the dependency version (to 4.5.5) which also meant that all the catch clauses were now unknown instead of any introduced in typescript@4.4. So for the code to transpile correctly, we had to type all those exceptions as any which is not great, but it's the best we can do for now. This can be fixed in a followup PR if someone is up for it.

CI is now green which will allow all other PRs to get merged.

@kobi-co kobi-co requested a review from a team February 6, 2023 10:38
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 6, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

package.json Outdated Show resolved Hide resolved
@dyladan
Copy link
Member

dyladan commented Feb 6, 2023

Looks like this isn't supported in node 14. Another possibility is to drop mongo v3 right?

@blumamir
Copy link
Member

blumamir commented Feb 6, 2023

Looks like this isn't supported in node 14. Another possibility is to drop mongo v3 right?

Drop the support from the instrumentation or just not run the test?

Looks like mongo latest 3 - 3.7.3 is downloaded in npm 1M times a week (more than v4)

@kobi-co
Copy link
Contributor Author

kobi-co commented Feb 6, 2023

Looks like this isn't supported in node 14. Another possibility is to drop mongo v3 right?

The only other option I see besides that is to patch @types/mongodb somehow and change the bson dependency there to a valid version instead of *.
Which seems quite difficult as this module already been deleted from DefinitelyTyped.
Tried my luck here: DefinitelyTyped/DefinitelyTyped#64241

@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

Merging #1374 (9459979) into main (aae03f2) will decrease coverage by 3.36%.
The diff coverage is 100.00%.

❗ Current head 9459979 differs from pull request most recent head 5e2e586. Consider uploading reports for the commit 5e2e586 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1374      +/-   ##
==========================================
- Coverage   96.08%   92.72%   -3.36%     
==========================================
  Files          14      133     +119     
  Lines         893     6639    +5746     
  Branches      191     1307    +1116     
==========================================
+ Hits          858     6156    +5298     
- Misses         35      483     +448     
Impacted Files Coverage Δ
...detector-aws/src/detectors/AwsBeanstalkDetector.ts 95.65% <ø> (ø)
...ource-detector-aws/src/detectors/AwsEksDetector.ts 91.25% <ø> (ø)
...etapackages/auto-instrumentations-web/src/utils.ts 95.83% <ø> (ø)
...ode/opentelemetry-instrumentation-dns/src/utils.ts 97.95% <ø> (ø)
...emetry-instrumentation-hapi/src/instrumentation.ts 99.31% <ø> (ø)
...try-instrumentation-ioredis/src/instrumentation.ts 91.86% <ø> (ø)
...lemetry-instrumentation-koa/src/instrumentation.ts 97.70% <ø> (ø)
...instrumentation-nestjs-core/src/instrumentation.ts 95.65% <ø> (ø)
...e/opentelemetry-instrumentation-redis/src/utils.ts 93.10% <ø> (ø)
...try-instrumentation-restify/src/instrumentation.ts 90.51% <ø> (ø)
... and 121 more

@blumamir
Copy link
Member

blumamir commented Feb 7, 2023

@open-telemetry/javascript-approvers @open-telemetry/javascript-maintainers since our build is currently broken, please have a look so we can merge fast. Thanks!

@blumamir blumamir changed the title chore(mongodb): fix build after v5 release chore: fix contrib build Feb 7, 2023
@dyladan
Copy link
Member

dyladan commented Feb 7, 2023

Please note that this will raise the minimum required TS version to 4.4 because of a change in TS 4.4 which affects the output that causes older versions to fail to read it. The SDK and API already have 4.4.4 so this should be no impact to end users.

In a future SIG meeting we will discuss a more general policy for upgrading typescript (not this week or next due to maintainer vacations).

@dyladan dyladan merged commit 4e3f0c2 into open-telemetry:main Feb 7, 2023
@kobi-co kobi-co deleted the mongodb-types branch February 7, 2023 14:35
@wbt
Copy link

wbt commented Feb 7, 2023

The referenced PR has been merged into logform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.