feat(instr-tedious): added support for v18 #2267

kirrg001 · 2024-06-07T10:13:22Z

closes #2266

Why?

Tedious v18 moved @types/tedious to their repository.
Tedious v18 uses Typescript v5 and es2022

Problems:

We need to use different typescript versions in the project. This creates conflicts and errors. I think because by default TS checks all TS Definitions in the root folder, which are not compatible against the TS v5.
We are using an outdated module ts-mocha. Won't build with newer typescript targets.
NPM Workspaces do not support nohoisting. Using nohoist makes it possible to move all @types/* and typescript dependencies into the packages and would avoid errors we are getting from typescript regarding root dependencies.
Eslint packages no longer compatibale. We had to update eslint packages for typescript too (for the package)
I had to use ts-ignore flag to import different type notations into the test. Not optimal, but easiest way. And using dynamic imports based on the tedious version did not work for me. And doing it like mongo is soo much code duplications. But I am also not an expert in TS. I am open to full working solutions.

Possible follow up tickets:

There is a root typescript dependency and every sub package has its own TS version. That doesn't make sense to me. Either one root ts version which every packages use and a package can override it on its own or the other way around.
Use tilde for dev dependencies to speed up installation (consistency change).
Replace ts-mocha with ts-node everywhere.
Update mocha to latest version everywhere.
Find a better way to avoid using ts-ignore.

codecov · 2024-06-07T13:04:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.29%. Comparing base (dfb2dff) to head (c007be8).
Report is 210 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2267      +/-   ##
==========================================
- Coverage   90.97%   90.29%   -0.69%     
==========================================
  Files         146      147       +1     
  Lines        7492     7242     -250     
  Branches     1502     1499       -3     
==========================================
- Hits         6816     6539     -277     
- Misses        676      703      +27

Files	Coverage Δ
...ode/instrumentation-tedious/src/instrumentation.ts	`92.13% <ø> (-0.34%)`	⬇️

... and 57 files with indirect coverage changes

closes open-telemetry#2266

kirrg001 · 2024-06-12T13:45:14Z

plugins/node/instrumentation-tedious/package.json

@@ -47,20 +47,23 @@
    "@opentelemetry/api": "^1.3.0"
  },
  "devDependencies": {
+    "@typescript-eslint/eslint-plugin": "6.21.0",
+    "@typescript-eslint/parser": "6.21.0",
+    "eslint": "8.9.0",


8.9.0 includes support for es2022.

We do not want to go higher as part of this PR, because it raises more errors e.g. --ext is not defined. Bumping eslint to the latest can be part of a different PR.

kirrg001 · 2024-06-12T13:47:34Z

plugins/node/instrumentation-tedious/package.json

@@ -47,20 +47,23 @@
    "@opentelemetry/api": "^1.3.0"
  },
  "devDependencies": {
+    "@typescript-eslint/eslint-plugin": "6.21.0",


v6 is still compatible with eslint v8 and adds support for es2022 as well.

kirrg001 · 2024-06-12T13:48:25Z

plugins/node/instrumentation-tedious/package.json

    "test-all-versions": "6.1.0",
-    "ts-mocha": "10.0.0",
-    "typescript": "4.4.4"
+    "ts-node": "^10.9.2",


Replaced ts-mocha. Was no longer working.

kirrg001 · 2024-06-12T13:51:10Z

plugins/node/instrumentation-tedious/test/api.ts

+  ConnectionConfig as ConnectionConfigLegacy,
+  // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+  // @ts-ignore
+  ConnectionConfiguration as ConnectionConfigNew,


I know using ts-ignore is not ideal. I was not able to find a better & easier solution. And I am not very familiar with TS.

Possible long-term:

write tests in TS

run tests in JS

requirement: remove the test build folder from the git ignore list

needs a follow up ticket IMO

kirrg001 · 2024-06-12T13:54:06Z

eslint.config.js

@@ -31,6 +31,8 @@ module.exports = {
      rules: {
        "@typescript-eslint/no-floating-promises": "error",
        "@typescript-eslint/no-this-alias": "off",
+        '@typescript-eslint/no-explicit-any': "warn",
+        '@typescript-eslint/no-unused-vars': "warn",


typescript-eslint/typescript-eslint@5346b5b

We need to add these because of using typescript-eslint/typescript-eslint in this pkg.
I know they need to belong to the eslint package file, but I thought this is fine as well and easier, because it was a warning before anyway.

kirrg001 · 2024-06-12T13:56:58Z

RFR

There were lots of challenges in this PR. I have tried my best to give context for the changes.

kirrg001 · 2024-06-18T11:17:16Z

@open-telemetry/javascript-approvers Requesting a review

blumamir

Thank you for working on adding the support for this new version.

Wrote few comments / concerns

blumamir · 2024-06-21T15:00:38Z

plugins/node/instrumentation-tedious/src/instrumentation.ts

@@ -78,7 +78,7 @@ export class TediousInstrumentation extends InstrumentationBase {
    return [
      new InstrumentationNodeModuleDefinition(
        TediousInstrumentation.COMPONENT,
-        ['>=1.11.0 <18'],
+        ['>=1.11.0 <19'],


please also update the supported version range in the README of the instrumentation.

blumamir · 2024-06-21T15:01:47Z

plugins/node/instrumentation-tedious/package.json

-    "ts-mocha": "10.0.0",
-    "typescript": "4.4.4"
+    "ts-node": "^10.9.2",
+    "typescript": "5.3.3"


I think this might be considered a break change which we try to avoid. @dyladan is it safe to apply?

Why is that a breaking change? Its a dev dependency.

Please respect the reasoning in the PR description. We have to update ts

I am not familiar with the exact details, but I remember discussions that if we update typescript version for the package, exiting users of the instrumentation with typescript 4 might have their transpiration broken.
@open-telemetry/javascript-maintainers can someone please help and supply guidance on the safe way to do that?

blumamir · 2024-06-21T15:03:43Z

plugins/node/instrumentation-tedious/package.json

    "@opentelemetry/api": "^1.3.0",
    "@opentelemetry/context-async-hooks": "^1.8.0",
    "@opentelemetry/contrib-test-utils": "^0.40.0",
    "@opentelemetry/sdk-trace-base": "^1.8.0",
    "@types/mocha": "7.0.2",
    "@types/node": "18.6.5",
-    "mocha": "7.2.0",
+    "mocha": "^10.4.0",


is it needed for this PR to work?
we aim to align the entire repo to use the same common dependencies, so this should ideally be bumped as a dedicated PR that does that for all packages in the repo

I understand. As far as I remember I had to update mocha because I had to replace mocha-ts with ts-node as described.

But I can double check if you want to 👌

Thank you, @kirrg001.

I reviewed a few other instrumentations to see how they handle tests. Based on their dependencies, I believe our current setup should work without needing to bump the shared monorepo dependency versions unless absolutely necessary.

However, I do support the idea of opening a dedicated PR to update some of the dependencies across the repository. If you have the time and would like to contribute to this effort, it would be greatly appreciated.

I will try to downgrade mocha to v7 again and let you know :)

blumamir · 2024-06-21T15:07:14Z

plugins/node/instrumentation-tedious/test/api.ts

+  // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+  // @ts-ignore
+  ConnectionConfiguration as ConnectionConfigNew,
+} from 'tedious';


Since we have both types from tedious and from the still existing @types/tedious dependency, is this import able to join them both?

I am not aware of any other solution. Please suggest a working alternative approach :)

I was actually wondering if you know how it works. Since it's test code, i don't think we need to worry and the current implementation is fine with me, but curious how typescript handles situation where the types exists twice in node_modules

If the tests are running, depending on the current installed tedious version (types are either in tedious or in @types), the code imports whats available.

I feel like TS was not designed to fulfill such a use case.

kirrg001 · 2024-07-18T08:08:05Z

@open-telemetry/javascript-approvers Requesting a review

kirrg001 · 2024-07-23T10:22:20Z

@open-telemetry/javascript-maintainers I would love to merge this. Requesting further feedback.

trentm · 2024-08-10T01:17:01Z

Hi. I took a look and tried to work through the same issues. I put up a branch for comparison:
main...trentm:opentelemetry-js-contrib:tm-tedious-18-play
Repeating its commit message (because GitHub likes to hide those):

    tedious@18 adds its own .d.ts files. Compiling with them requires
    TypeScript v5, and either target:'es2022' for 'AggregateError'
    or using skipLibCheck:true in tsconfig.json.

    This change limits the update to TypeScript v5 to just while
    testing tedious@18 via test-all-versions' support for
    'peerDependencies'.

    THis also drops ts-mocha in favour of 'mocha --require ts-node/register'
    as was done on opentelemetry-js.git in https://github.com/open-telemetry/opentelemetry-js/pull/4840

It uses some of the work that you've done, and tries to limit some things (like not updating the typescript version in package.json).
I agree that TypeScript v5 is required to compile the test files with tedious@18. Given that we only need to do that for test-all-versions (TAV) testing, I used peerDependencies in ".tav.yml" for that (https://github.com/watson/test-all-versions?tab=readme-ov-file#peer-dependencies).
I limited some of the needed type changes in test/api.ts.
I also split out some of the versions in .tav.yml for minimum support Node.js versions. This wasn't completely necesasry because test/instrumentation.test.ts knows how to skip out if the current version of Node and tedious isn't supported.
I'm good with the switch to mocha --require ts-node/register. This was done a few months back in the opentelemetry-js.git repo. We should follow-up with a separate change to do the same for all the other packages in this repo.

on skipLibCheck

I found that skipLibCheck: true in "tsconfig.json" was necessary, otherwise the test code won't compile with tedious@18.

It would fail with:

../../../node_modules/tedious/lib/connection.d.ts:761:29 - error TS2304: Cannot find name 'AggregateError'.

761     loginError: undefined | AggregateError | ConnectionError;
                                ~~~~~~~~~~~~~~

The alternative would be to update the tsconfig "target" to "es2022". That might have an impact on the shipping code for instr-tedious, so I thought that should be avoided for now, at least.

options

Please let me know which you would prefer:

I could make a PR out of my branch for discussion and see about pushing that through, or
You could take the few changes I have from yours. If you do take my changes, please take my (smaller) diff to the top-level package-lock.json file as well.

kirrg001 · 2024-08-12T07:45:41Z

@trentm Thanks so much for taking the time to review this.

I could make a PR out of my branch for discussion and see about pushing that through, or

This option sounds like the easiest. I'd suggest that you are opening your PR and I'll close mine :)

kirrg001 · 2024-08-15T12:25:56Z

Closing in favor of #2381

github-actions bot added the pkg:instrumentation-tedious label Jun 7, 2024

github-actions bot assigned rauno56 Jun 7, 2024

github-actions bot requested a review from rauno56 June 7, 2024 10:13

kirrg001 force-pushed the tediousv18 branch from 9dea514 to f11a85e Compare June 7, 2024 12:55

kirrg001 force-pushed the tediousv18 branch 4 times, most recently from 1696fa7 to ed80e2d Compare June 10, 2024 10:09

kirrg001 added 3 commits June 12, 2024 15:08

feat(instr-tedious): added support for v18

8df5740

closes open-telemetry#2266

chore: re-added @types/tedious

98f20b8

test: make it possible to run all tests and versions

79e9409

kirrg001 force-pushed the tediousv18 branch from 21e4dc9 to 79e9409 Compare June 12, 2024 13:08

chore: fix lint

c007be8

kirrg001 commented Jun 12, 2024

View reviewed changes

kirrg001 marked this pull request as ready for review June 12, 2024 13:54

kirrg001 requested a review from a team June 12, 2024 13:54

blumamir reviewed Jun 21, 2024

View reviewed changes

trentm mentioned this pull request Aug 14, 2024

feat(instrumentation-tedious): support tedious@18 #2381

Merged

kirrg001 closed this Aug 15, 2024

trentm mentioned this pull request Aug 23, 2024

chore(deps): update dependency tedious to v19 #2400

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(instr-tedious): added support for v18 #2267

feat(instr-tedious): added support for v18 #2267

kirrg001 commented Jun 7, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading

kirrg001 Jun 12, 2024 •

edited

Loading

kirrg001 Jun 12, 2024

kirrg001 Jun 12, 2024

kirrg001 Jun 12, 2024 •

edited

Loading

kirrg001 Jun 12, 2024

kirrg001 commented Jun 12, 2024

kirrg001 commented Jun 18, 2024

blumamir left a comment

blumamir Jun 21, 2024

kirrg001 Jun 21, 2024

blumamir Jun 21, 2024

kirrg001 Jun 21, 2024

blumamir Jun 22, 2024

blumamir Jun 21, 2024

kirrg001 Jun 21, 2024

blumamir Jun 22, 2024

kirrg001 Jun 22, 2024

blumamir Jun 21, 2024

kirrg001 Jun 21, 2024

blumamir Jun 22, 2024

kirrg001 Jun 22, 2024

kirrg001 commented Jul 18, 2024

kirrg001 commented Jul 23, 2024

trentm commented Aug 10, 2024

kirrg001 commented Aug 12, 2024 •

edited

Loading

kirrg001 commented Aug 15, 2024

feat(instr-tedious): added support for v18 #2267

feat(instr-tedious): added support for v18 #2267

Conversation

kirrg001 commented Jun 7, 2024 • edited Loading

codecov bot commented Jun 7, 2024 • edited Loading

Codecov Report

kirrg001 Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirrg001 Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirrg001 commented Jun 12, 2024

kirrg001 commented Jun 18, 2024

blumamir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirrg001 commented Jul 18, 2024

kirrg001 commented Jul 23, 2024

trentm commented Aug 10, 2024

on skipLibCheck

options

kirrg001 commented Aug 12, 2024 • edited Loading

kirrg001 commented Aug 15, 2024

kirrg001 commented Jun 7, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading

kirrg001 Jun 12, 2024 •

edited

Loading

kirrg001 Jun 12, 2024 •

edited

Loading

kirrg001 commented Aug 12, 2024 •

edited

Loading