feat: allow extending BasicTracerProvider with custom registered components

[rauno56]

Which problem is this PR solving?

Extending BasicTracerProvider still takes registered propagators and exporters from the static members of BasicTracerProvider, not the child class.
Typing is not ideal, TS doesn't support this usecase afaik, but I think it's safe to assume the types will stay the same in the hierarchy tree so I'm casting it to whatever the respective BasicTracerProvider member is.

Short description of the changes

Fix by dynamically referencing _registeredPropagators and _registeredExporters from the class. Extender might still want to override the getter, but only if they actually want to change the logic which is used, and not when they only change the Map.

Also tidied up some of the tests while I was at it and added tests for the old use-case to make sure that extending works for people whichever way they do it.

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[codecov]

Codecov Report

Merging #3023 (0202c35) into main (1e8b896) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 0202c35 differs from pull request most recent head ea6dd7e. Consider uploading reports for the commit ea6dd7e to get more accurate results

@@            Coverage Diff             @@
##             main    #3023      +/-   ##
==========================================
+ Coverage   93.06%   93.08%   +0.01%     
==========================================
  Files         188      188              
  Lines        6248     6246       -2     
  Branches     1313     1313              
==========================================
- Hits         5815     5814       -1     
+ Misses        433      432       -1     

Impacted Files	Coverage Δ
...telemetry-sdk-trace-node/src/NodeTracerProvider.ts	95.00% <ø> (-0.84%)	⬇️
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts	95.49% <100.00%> (+1.00%)	⬆️

[vmarchaud]

I'm not sure to understand your use case, you should be able to add your exporters/propagators in _registeredPropagators within your extanded class and everything should works ?

[rauno56]

I'm not sure to understand your use case, you should be able to add your exporters/propagators in _registeredPropagators within your extanded class and everything should works ?

That's exactly the idea, yes.

[vmarchaud]

That's exactly the idea, yes.

Okay but that's exactly what the current implementation allows, i don't see why you need to change anything ^^

[rauno56]

Well, why does NodeTracerProvider have to then override _registeredPropagators and _getPropagator then?

Because of the hardcoded usage of BasicTracerProvider in the getter any child class would also need to override _getPropagator to bypass only checking BasicTracerProvider's list of registered propagators. This either renders having a static list utterly pointless or mean that the getter implementation in BasicTracerProvider has a bug. I'm assuming the latter. NodeTracerProvider's is a sign of that. I'll make sure to have some tests as well like we discussed in the SIG, hence the draft status.

[vmarchaud]

Well, why does NodeTracerProvider have to then override _registeredPropagators and _getPropagator then?

Because i though it was fine to override two both at the time, i agree that it's possible to simplify and only have to override _registeredPropagators but i initially understood that you were saying that the implementation didnt work

[rauno56]

i initially understood that you were saying that the implementation didnt work

My bad - The implementation did technically work, but required overrides in two locations which was the point for my confusion.

Unfortunately the simplification did require more work than I expected to ensure the backwards compatibility. Hopefully we can remove that trickery at some point.

[legendecas]

Is this work still a draft?

Sorry, no idea why GitHub is still displaying the PR as a draft. I tried to force refresh the page but it didn't work out correctly.

[legendecas]

LGTM % one nit

[legendecas]

@rauno56 friendly ping -- any updates on this?

[rauno56]

Thanks for the reminder! Addressed all the comments, good to go once the CI is green.

[vmarchaud]

@blumamir since you had comments on this could you check it again and merge if it looks good to you ?