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

fix(instrumentation-mongodb): update mongodb to version without missing types #1036

Closed

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented May 24, 2022

Fixes #1031

@dyladan dyladan requested a review from a team May 24, 2022 17:28
@dyladan dyladan changed the title feat(instrumentation-mongodb): update mongodb fix(instrumentation-mongodb): update mongodb to version without missing types May 24, 2022
@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #1036 (8ecb0a5) into main (1da0216) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1036   +/-   ##
=======================================
  Coverage   95.91%   95.91%           
=======================================
  Files          13       13           
  Lines         856      856           
  Branches      178      178           
=======================================
  Hits          821      821           
  Misses         35       35           

@dyladan dyladan marked this pull request as draft May 24, 2022 18:08
@@ -58,7 +58,7 @@
"@types/node": "16.11.21",
"gts": "3.1.0",
"mocha": "7.2.0",
"mongodb": "3.6.11",
"mongodb": "4.6.0",
Copy link
Member

Choose a reason for hiding this comment

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

Having mongodb in dev deps only will result in missing types for consumers.
Adding a specific mongodb version as dependency seems not good.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. thats why this is a draft. i'm trying to figure out other options. Unfortunately, I think we're going to have to stop trying to type check modules we're patching. causing too many dependency troubles.

Copy link
Member

Choose a reason for hiding this comment

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

For the patching part it's quite simple as the instrumentation base classes don't need/use the types at all.
The problem are the hooks offered by some instrumentations as they are APIs to end users. We could use unknown/any there and just document what is provided and user as to cast. This would even cover differences between versions.

@@ -58,7 +58,7 @@
"@types/node": "16.11.21",
"gts": "3.1.0",
"mocha": "7.2.0",
"mongodb": "3.6.11",
"mongodb": "4.6.0",
Copy link
Member

Choose a reason for hiding this comment

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

This is deliberately set to version 3 as it is what we run in CI when invoking npm run test:

    "test": "nyc ts-mocha -p tsconfig.json --require '@opentelemetry/contrib-test-utils' 'test/**/mongodb-v3.test.ts'",

Since we can only run a single version, version 3 was choosen as it is more common

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2022

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Aug 1, 2022
@dyladan dyladan closed this Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken auto-instrumentation dependencies
3 participants