fix!: export instrumentations only as named export

[blumamir]

This PR is targets how we name and export instrumentation classes from our packages across the repo, and introduces consistency and best practices to align the code base.

It makes it so that:

• each instrumentation class has the same name as the name it is exported with (few were called Instrumentation instead of FooInstrumentation)
• align the index.ts file to export consistently - everything from instrumentation.ts and type.ts. This is to remove complexity and promote safe practices. This aligns the few places which were too complex, to an equivalent import and export statements to align with most packages in the repo.
• Remove the default export of the instrumentation class from the few places where it was present. since all instrumentations supports named exports of the class, I assume very few people if any are actually importing an instrumentation this way, but it is possible however, and those users will have their transplation broken when upgrading to the new version and will need to update the import.

Benefits

• Consistency across the various packages in the repo - better maintenance and developing experince.
• Shorter code - remove some of the un-needed complexities in the index.ts files and replace with shorter equivalents.
• better practices - the types.ts and instrumentation.ts files are intended to be user facing, and only export what the user is expected to consume. More on that in the GUIDELINES. Exporting everything from those files will prevent accidentally forgetting to add new types to the index.ts as happen in the past

Breaking Change

This PR is a breaking change by removing the default export for the following instrumentations:

• fs
• lru-memoizer
• tedious
• generic-pool
• knex
• memcahed
• mysql2
• restify
• router

As state above, it is possible breaking change, but very unlikely for real users as this style is supported for only few instrumentations. It will be published as minor patch which should not be pulled automatically by users that has caret dependency, so they need to actively pull new version and will observe the transpilation failing.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.30%. Comparing base (dfb2dff) to head (cb60059).
Report is 173 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2296      +/-   ##
==========================================
- Coverage   90.97%   90.30%   -0.68%     
==========================================
  Files         146      147       +1     
  Lines        7492     7263     -229     
  Branches     1502     1509       +7     
==========================================
- Hits         6816     6559     -257     
- Misses        676      704      +28     

Files	Coverage Δ
...nstrumentation-generic-pool/src/instrumentation.ts	40.54% <100.00%> (+0.54%)	⬆️
...y-instrumentation-memcached/src/instrumentation.ts	96.22% <100.00%> (-0.21%)	⬇️
...instrumentation-nestjs-core/src/instrumentation.ts	95.45% <100.00%> (-0.20%)	⬇️
...etry-instrumentation-router/src/instrumentation.ts	95.89% <100.00%> (-0.27%)	⬇️

... and 55 files with indirect coverage changes

[pichlermarc]

Thanks for taking care of this. 🙂

As far as aligning export goes, I wonder if we should actually align them to be explicit exports (instead of wildcard exports) wherever possible. It has happened often that we accidentally add to the public interface by adding something to a file which in index.ts was then re-exported by a wildcard export.

We can avoid these accidental exports by using explicit exports wherever we can (see also: open-telemetry/opentelemetry-js#4186)

[blumamir]

Thanks for taking care of this. 🙂

As far as aligning export goes, I wonder if we should actually align them to be explicit exports (instead of wildcard exports) wherever possible. It has happened often that we accidentally add to the public interface by adding something to a file which in index.ts was then re-exported by a wildcard export.

We can avoid these accidental exports by using explicit exports wherever we can (see also: open-telemetry/opentelemetry-js#4186)

Thank you for the review

This issue has a bit of history behind it:

• up until fix: separate public and internal types for all instrumentations #1251 , the types.ts files contained both public facing exports, and internal exports.
• one can potentially mix public and private exports into the same file, and apply explicit exports from index.ts as suggested
• private types often rely on importing dev dependency, like the instrumented package.
• if an import from dev dependency is found in a .d.ts file, it will fail transpilation for the instrumentation package users. which happened very often in the past.
• types.ts and instrumentation.ts both contains user facing exports, thus the user transpiler will for sure need to import both types.d.ts and index.d.ts and all there transitive dependencies
• It was agreed that both these files MUST NOT contain any private exports, due to how easy it is for dev dependencies to leak into the public api with private exports, even with explicit exports.
• Given the last comment, if all exports from types.ts and instrumentation.ts are public, then exporting them as wildcard is safe.
• using explicit exports can makes it easier to forget exporting a type, which has happened frequently in the past.

All these decisions where made to prevent painful issues which had happen. I am open to discuss alternatives but personally think that using wildcard exports is the safest option, which also communicate the intent of these files to code readers / future maintainers, and reduces the chance of forgetting to export something important or exporting dev dependencies in the public api.

[pichlermarc]

I see, thanks for the detailed response. I think in light of that info aligning it the way you did is the correct way then. 🙂

[trentm]

Didn't we actually want to move away from export * ... if we didn't need it? open-telemetry/opentelemetry-js#4186