chore: avoid leaking type definitions in instrumentations

[Flarna]

Currently instrumentations leak the type of their host modules via the protected init() function required by generic base class InstrumentationAbstract.

Actually the instrumentation module has no need for a concrete type here. Therefore replaced the generic type by unknown.

In the instrumentation the return type of init() is set explicit to InstrumentationNodeModuleDefinition<any>[] to avoid leaking of types. The creation of the InstrumentationNodeModuleDefinition instances is still using the concrete types to ensure proper type checking within the implementation of the instrumentations.

Besides that I made some protected functions of HTTP instrumentation private since they are no longer needed as http and https is combined meanwhile.

Leaking type definition required to add @types packages as dependencies. These @types packages in turn may require real packages (for example installing @opentelemetry/instrumentation-graphql results in graphql installed as @types/graphql depends on graphql itself).

Refs: open-telemetry/opentelemetry-js-contrib#460
Refs: open-telemetry/opentelemetry-js-contrib#468

[codecov]

Codecov Report

Merging #2256 (b4b31dc) into main (b3d3d86) will not change coverage.
The diff coverage is 85.71%.

@@           Coverage Diff           @@
##             main    #2256   +/-   ##
=======================================
  Coverage   92.77%   92.77%           
=======================================
  Files         145      145           
  Lines        5216     5216           
  Branches     1068     1068           
=======================================
  Hits         4839     4839           
  Misses        377      377           

Impacted Files	Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	96.91% <50.00%> (ø)
...emetry-instrumentation-xml-http-request/src/xhr.ts	97.58% <50.00%> (ø)
...elemetry-instrumentation-grpc/src/grpc-js/index.ts	90.81% <100.00%> (ø)
...entelemetry-instrumentation-grpc/src/grpc/index.ts	93.04% <100.00%> (ø)
...ges/opentelemetry-instrumentation-http/src/http.ts	94.90% <100.00%> (ø)
...entelemetry-instrumentation/src/instrumentation.ts	70.37% <100.00%> (ø)
...strumentation/src/platform/node/instrumentation.ts	23.61% <100.00%> (ø)

[MSNev]

My Preference would be to keep the type so that any user can retain the type safety.

It seems to me that what we should do is (where necessary) to have the instrumentation export a "public" interface, any if the specific instrumentation doesn't want to do that, it can use "any" or "unknown".

[Flarna]

The type which was used here had no impact on the concrete instrumentation. The concrete type to use was (and still is) specified in the concrete implementation to instantiate the InstrumentationNodeModuleDefinitions.

Maybe we could keep the <T = any> here but I see no value in it. Just take a look at e.g. xhr instrumenation. It has to specifies a type which is not used anywhere afterwards.

[MSNev]

But, for any instrumentation where they want to support their own extensions (properties or functions) this would provide additional type safety and they can choose to still hide the actual internal workings (from type safety / code completion / documentation / exported types) but provide some sort of public API etc.

[MSNev]

I agree, that specifying any doesn't have any advantages for what I said above. As you are basically saying that there is no type safety and do what you want. (and yes I know that is pure JS you can anyway)

[Flarna]

Users can (and should) still use concrete types when creating a InstrumentationNodeModuleDefinition.
They can even make their class extending InstrumentationAbstract a generic and supply a type if this helps anywhere.

But within the @opentelemetry/instrumentation module the type has no value for type safety to my understanding.

[MSNev]

Agree the type has Zero benefit for the internal workings, it's purely for consumers.

[obecny]

Originally the types were added to keep consistency to what we had previously - plugins had defined type too. Some of the instrumentations already uses mixed types and quite often the last thing you can do is to use any. If that would also help to resolve issue with having to include types for the package in dependencies I would be fully into removing this anyway then.

[dyladan]

It seems to me this conversation is resolved no?

[Flarna]

actually this is the change really required to avoid leaking grpc types here.

[MSNev]

And this is where the "generic" (interface) should be used (I may be over simplifying)
protected init(): InstrumentationNodeModuleDefinition<T>[] {

[Flarna]

init() is called by base class constructor (and this is the only one who should call init) and base class has no need for the types.
But generated type definitions hold full types for public and protected methods therefore this causes that types leak and we have to add the @types packages as production dependencies (see open-telemetry/opentelemetry-js-contrib#460 for more details).

Moving init() to private would solve this also but this is not allowed in typescript (it would be in C++...).

[Flarna]

Looking into e.g. open-telemetry/opentelemetry-js-contrib#527 I fear it's anyway not easily possible to avoid that we leak types in general.

Even without this PR a concrete instrumentation can use return type InstrumentationNodeModuleDefinition<any>[] to avoid leaking types because of the non end-user facing init().
But config is for end users so either specify a fresh type or leak. Using any or unknown there would be not good.

[vmarchaud]

Looking into e.g. open-telemetry/opentelemetry-js-contrib#527 I fear it's anyway not easily possible to avoid that we leak types in general.

I'm not sure to understand the status of the PR then, do we want to continue with this ?

[Flarna]

Closing as leaking types happens also because of other reasons (request/response hooks,...).