Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow extending BasicTracerProvider with custom registered components #3023

Merged
merged 16 commits into from
Jul 20, 2022

Conversation

rauno56
Copy link
Member

@rauno56 rauno56 commented Jun 7, 2022

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

@rauno56 rauno56 requested a review from a team June 7, 2022 12:46
@codecov
Copy link

codecov bot commented Jun 7, 2022

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%) ⬆️

@rauno56 rauno56 marked this pull request as draft June 7, 2022 13:09
@vmarchaud
Copy link
Member

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
Copy link
Member Author

rauno56 commented Jun 9, 2022

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
Copy link
Member

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
Copy link
Member Author

rauno56 commented Jun 11, 2022

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.

@rauno56 rauno56 marked this pull request as ready for review June 14, 2022 16:10
@vmarchaud
Copy link
Member

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

@vmarchaud vmarchaud requested a review from a team June 15, 2022 12:05
@rauno56
Copy link
Member Author

rauno56 commented Jun 15, 2022

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
Copy link
Member

legendecas commented Jun 21, 2022

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.

image

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % one nit

@legendecas
Copy link
Member

@rauno56 friendly ping -- any updates on this?

@rauno56
Copy link
Member Author

rauno56 commented Jul 12, 2022

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

@vmarchaud
Copy link
Member

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

@vmarchaud vmarchaud requested a review from blumamir July 15, 2022 09:09
@dyladan dyladan merged commit 69cf899 into open-telemetry:main Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants