feat!: more specific types for getConfig and setConfig

[dyladan]

Fixes #2258 Should not require any change to existing instrumentations, but allows instrumentations to declare config types with more properties which are used by config get/set

[codecov]

Codecov Report

Merging #2388 (00c4864) into main (3b9cd16) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2388      +/-   ##
==========================================
- Coverage   92.78%   92.64%   -0.14%     
==========================================
  Files         145      142       -3     
  Lines        5226     5101     -125     
  Branches     1071     1048      -23     
==========================================
- Hits         4849     4726     -123     
+ Misses        377      375       -2     

Impacted Files	Coverage Δ
...entelemetry-instrumentation/src/instrumentation.ts	70.37% <100.00%> (ø)
...ontext-async-hooks/src/AsyncHooksContextManager.ts
...sync-hooks/src/AbstractAsyncHooksContextManager.ts
...async-hooks/src/AsyncLocalStorageContextManager.ts

[Flarna]

It would be nice to adapt one of the instrumentations like http to have an implicit test.

[dyladan]

@MSNev re-requested your review since the last change is breaking

[obecny]

would this work ?

Suggested change

	export abstract class InstrumentationAbstract<InstrumentedModuleType = any, Config = types.InstrumentationConfig>
	export abstract class InstrumentationAbstract<InstrumentedModuleType = any, Config extends types.InstrumentationConfig>

[dyladan]

If we make Config required we have to also make InstrumentedModuleType optional (required types can't follow optional types). Is that what you're suggesting?

[Flarna]

I think it should be a combination - optional with default to types.InstrumentationConfig but if a concrete type is specified it must extend types.InstrumentationConfig (otherwise it would be allowed to use e.g. number).

[obecny]

just a friendly ping here to prevent stuck

[Flarna]

I think it's needed to additionally adapt abstract class InstrumentationBase in packages\opentelemetry-instrumentation\src\platform\node\instrumentation.ts otherwise the change is not visible to instrumentations in node case.

Once again, if you could adapt at least one instrumentation like http we would have an implicit test for this.

[dyladan]

It is required. I've been wrestling with it all morning because the instrumentation loader doesn't like the parameterized types. It actually turns out it's been broken since forever because the InstrumentationBase doesn't pass the T type so it is always any.

[Flarna]

Well, it was not really broken. As indicated in #2256 the base class has no real need for T - it passes it through only. The relevant type checking happens only in the concrete class.

[github-actions]

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]

This PR was closed because it has been stale for 14 days with no activity.