fix: avoid type imports of the instrumented package in the built assets

[rauno56]

Which problem is this PR solving?

#992 explains the issue very well in the context of NestJS instrumentation.
It turns out we have the same issue with importing following packages.

• aws-sdk moved to fix: avoid type imports of the aws-sdk package in the built assets #1066
• cassandra-driver
• knex
• mysql2
• router

... and more that are not addressed in this PR.

To repro the issue one has to delete hoisted node_modules and do npm i in the metapackage.

Short description of the changes

1. InstrumentationBase and InstrumentationNodeModuleDefinition generics now use any,
2. Imported some types for aws-sdk and mysql2 implementation, how should I address any licensing topics?
3. made startSpan on cassandra instrumentation private,

See the issue referenced above.
I'm removing references to any types imported from the instrumented package from exported interfaces of the instrumentation.

[codecov]

Codecov Report

Merging #1017 (84d8420) into main (f9e81b0) will decrease coverage by 1.56%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1017      +/-   ##
==========================================
- Coverage   95.91%   94.34%   -1.57%     
==========================================
  Files          13       25      +12     
  Lines         856     1397     +541     
  Branches      178      280     +102     
==========================================
+ Hits          821     1318     +497     
- Misses         35       79      +44     

Impacted Files	Coverage Δ
.../opentelemetry-instrumentation-mysql2/src/utils.ts	100.00% <ø> (ø)
.../opentelemetry-instrumentation-router/src/utils.ts	100.00% <ø> (ø)
...y-instrumentation-cassandra/src/instrumentation.ts	80.37% <100.00%> (ø)
...emetry-instrumentation-knex/src/instrumentation.ts	98.78% <100.00%> (ø)
...etry-instrumentation-mysql2/src/instrumentation.ts	94.36% <100.00%> (ø)
...etry-instrumentation-router/src/instrumentation.ts	96.15% <100.00%> (ø)
...pentelemetry-instrumentation-knex/src/constants.ts	100.00% <0.00%> (ø)
... and 8 more

[rauno56]

Although I'm not sure we want to go through with this because of open-telemetry/opentelemetry-js#2256

[dyladan]

I suspect we will need to do this with all or most of our instrumentations, making the generic sort of pointless. Maybe we can vendor the types ourselves? That would be a lot of work though.

[Flarna]

The generic is not used at all by @opentelemetry/instrumenation therefore I'm quite sure it would be possible to refactor the instrumentation setup in a way to hide this from public interface.

But there are quite some instrumentations which offer hooks and these hooks use types as they pass data from the instrumented library via these hooks. We could use unknown or any there and let the typescript user cast.

[dyladan]

The generic is not used at all by @opentelemetry/instrumenation therefore I'm quite sure it would be possible to refactor the instrumentation setup in a way to hide this from public interface.

But there are quite some instrumentations which offer hooks and these hooks use types as they pass data from the instrumented library via these hooks. We could use unknown or any there and let the typescript user cast.

I agree unknown is probably the best option

[blumamir]

The generic is not used at all by @opentelemetry/instrumenation therefore I'm quite sure it would be possible to refactor the instrumentation setup in a way to hide this from public interface.

I support doing this

But there are quite some instrumentations which offer hooks and these hooks use types as they pass data from the instrumented library via these hooks. We could use unknown or any there and let the typescript user cast.

I think for types in hooks the best option is to defer responsibility to the library author:

• Use unknown or any with comments near the hook interface explaining on how to type if used.
• add a dependency on @types/foo if it only implements types and is a thin package.
• Copy relevant interfaces into the types.ts file to be exported from instrumentation.

[rauno56]

Any other comments, @Flarna, @dyladan? Any feedback on this and whether this is the way you intended it to go towards will be highly appreciated.

[dyladan]

The implementation looks good to me, but the licensing I think we need to be careful of for vendoring outside code (especially code from a company like AWS).

As an example, I looked into the aws-sdk-js types license. It is listed in https://github.com/aws/aws-sdk-js/blob/master/package.json#L87 as Apache-2.0

Full license available here: http://www.apache.org/licenses/LICENSE-2.0

The Apache-2.0 license allows redistribution of copyrighted works as long as the following conditions are met:

   4. Redistribution. You may reproduce and distribute copies of the
      Work or Derivative Works thereof in any medium, with or without
      modifications, and in Source or Object form, provided that You
      meet the following conditions:

      (a) You must give any other recipients of the Work or
          Derivative Works a copy of this License; and

      (b) You must cause any modified files to carry prominent notices
          stating that You changed the files; and

      (c) You must retain, in the Source form of any Derivative Works
          that You distribute, all copyright, patent, trademark, and
          attribution notices from the Source form of the Work,
          excluding those notices that do not pertain to any part of
          the Derivative Works; and

      (d) If the Work includes a "NOTICE" text file as part of its
          distribution, then any Derivative Works that You distribute must
          include a readable copy of the attribution notices contained
          within such NOTICE file, excluding those notices that do not
          pertain to any part of the Derivative Works, in at least one
          of the following places: within a NOTICE text file distributed
          as part of the Derivative Works; within the Source form or
          documentation, if provided along with the Derivative Works; or,
          within a display generated by the Derivative Works, if and
          wherever such third-party notices normally appear. The contents
          of the NOTICE file are for informational purposes only and
          do not modify the License. You may add Your own attribution
          notices within Derivative Works that You distribute, alongside
          or as an addendum to the NOTICE text from the Work, provided
          that such additional attribution notices cannot be construed
          as modifying the License.

      You may add Your own copyright statement to Your modifications and
      may provide additional or different license terms and conditions
      for use, reproduction, or distribution of Your modifications, or
      for any such Derivative Works as a whole, provided Your use,
      reproduction, and distribution of the Work otherwise complies with
      the conditions stated in this License.

It seems to me that we at least have to add a copyright header at the top of the file stating it is apache-2.0 licenced and has not (or has it?) been modified. We must also retain the NOTICE.txt file https://github.com/aws/aws-sdk-js/blob/master/NOTICE.txt although we are allowed to append our own wording to it.

Personally I would like to have someone with more legal knowledge look over it. Maybe we can get legal resources from the CNCF

[dyladan]

I reached out to the CNCF to see if we can get legal resources to ensure we're doing this correctly

[rauno56]

@dyladan, separating changes to aws code out from this PR for now then.
Let me know when you hear back from CNCF here: #1066

Sorry for the force-push... Removed the commits touching aws-sdk instrumentation from the history.

[dyladan]

Maybe in the future we can find some way to bring back a level of type safety without requiring dependencies for end users