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: tracers provided by the API become useable when provider registered #1448

Merged
merged 8 commits into from
Aug 27, 2020

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Aug 19, 2020

This is important to fix the load order issue as it relates to plugin setup. Currently, plugins can't be set up until after the tracer provider is created, and creating the tracer provider starts the resource detectors. If a resource detector like GCP uses a module that needs to be instrumented, the plugin may not be able to instrument it if it is loaded after the resource detectors run.

After this PR, plugins can be set up before the tracer provider. When they are constructed, they will acquire Proxy Tracers from the API. Then, when the tracer provider is registered, those proxy tracers become fully useable without the plugin having to acquire a new tracer.

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

Looks great!

packages/opentelemetry-api/src/api/trace.ts Outdated Show resolved Hide resolved
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

I'm missing the whole picture, how the new proxy classes should be used outside of api. I checked the draft for new base plugin and they are not used there, is this still in progress ? If exporting them is intentional where are they going to be used yet and how? thx

@dyladan
Copy link
Member Author

dyladan commented Aug 26, 2020

I'm missing the whole picture, how the new proxy classes should be used outside of api.

The basic idea is that if a library has built-in instrumentation, we will not have as much control over load order. We might have the case where a library is required and it calls getTracer before the SDK is initialized. In that case, the tracer the library has acquired will become useable when the SDK is registered without that library having to call getTracer again.

I checked the draft for new base plugin and they are not used there, is this still in progress ? If exporting them is intentional where are they going to be used yet and how? thx

The API is used the exact same way by users and plugins with no change. The only difference is that if you call getTracer before the SDK is initialized, the API will return a ProxyTracer which proxies to a noop tracer instead of a noop tracer directly. The proxy classes should probably not be exported. I will remove those exports.

@obecny
Copy link
Member

obecny commented Aug 26, 2020

@dyladan it could be merged once you resolve the conflicts

@dyladan
Copy link
Member Author

dyladan commented Aug 26, 2020

Build still needs to be fixed

@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #1448 into master will decrease coverage by 0.05%.
The diff coverage is 88.09%.

@@            Coverage Diff             @@
##           master    #1448      +/-   ##
==========================================
- Coverage   93.91%   93.85%   -0.06%     
==========================================
  Files         151      153       +2     
  Lines        4615     4656      +41     
  Branches      953      960       +7     
==========================================
+ Hits         4334     4370      +36     
- Misses        281      286       +5     
Impacted Files Coverage Δ
...ackages/opentelemetry-api/src/trace/ProxyTracer.ts 82.60% <82.60%> (ø)
...opentelemetry-api/src/trace/ProxyTracerProvider.ts 92.85% <92.85%> (ø)
packages/opentelemetry-api/src/api/trace.ts 92.30% <100.00%> (+1.39%) ⬆️

@dyladan dyladan merged commit 5c7753f into open-telemetry:master Aug 27, 2020
@dyladan dyladan deleted the proxy-tracer branch August 27, 2020 15:58
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants