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

tracers: store startup created tracers the same way as user created #316

Merged
merged 2 commits into from
Dec 13, 2021

Conversation

tsloughter
Copy link
Member

To simplify the understanding of tracer registration and fetching
this removes the separation between tracers registered on startup
for each application and those registered by a user or other code
at runtime.

Now there is a map to lookup the application name for a module
which is used to lookup the tracer, instead of before where the
map lookup actually got the tracer (meaning the tracer record was
duplicated and stored for every module!).

This means a user registering a tracer can override the tracer
registered at startup for an application name if they use the same
name, but I think this is sort of expected behavior so not an issue.

The change shouldn't effect much, if any, existing user code since
it almost always that tracers are fetched behind the scenes by
the macros.

@tsloughter tsloughter requested a review from a team November 24, 2021 20:46
To simplify the understanding of tracer registration and fetching
this removes the separation between tracers registered on startup
for each application and those registered by a user or other code
at runtime.

Now there is a map to lookup the application name for a module
which is used to lookup the tracer, instead of before where the
map lookup actually got the tracer (meaning the tracer record was
duplicated and stored for every module!).

This means a user registering a tracer can override the tracer
registered at startup for an application name if they use the same
name, but I think this is sort of expected behavior so not an issue.

The change shouldn't effect much, if any, existing user code since
it almost always that tracers are fetched behind the scenes by
the macros.
@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #316 (de3ecac) into main (bad9d42) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #316      +/-   ##
==========================================
- Coverage   38.60%   38.47%   -0.14%     
==========================================
  Files          50       50              
  Lines        3463     3462       -1     
==========================================
- Hits         1337     1332       -5     
- Misses       2126     2130       +4     
Flag Coverage Δ
api 64.39% <100.00%> (-0.44%) ⬇️
elixir 15.12% <30.00%> (+0.39%) ⬆️
erlang 38.49% <100.00%> (-0.14%) ⬇️
exporter 21.77% <ø> (ø)
sdk 76.17% <100.00%> (-0.32%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
apps/opentelemetry/src/opentelemetry_app.erl 100.00% <100.00%> (ø)
apps/opentelemetry_api/src/opentelemetry.erl 76.31% <100.00%> (+0.99%) ⬆️
...pps/opentelemetry_api/src/otel_tracer_provider.erl 62.50% <0.00%> (-18.75%) ⬇️
apps/opentelemetry/src/otel_tracer_server.erl 72.72% <0.00%> (-2.28%) ⬇️
apps/opentelemetry/src/otel_resource_detector.erl 91.89% <0.00%> (-1.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bad9d42...de3ecac. Read the comment docs.

@tsloughter
Copy link
Member Author

Last time this changes, I swear.

Originally it wasn't done this way because it requires a second lookup (first find the application name, then use that as the key for the persistent term where the tracer is stored). But I'd argue that those are fast enough and the user is able to store the returned tracer in a variable and use it if there are times they want to remove any lookup overhead in usage.

This extra lookup saves from having to store the tracer record duplicated for every single module and it removes the separation between tracers created by a user and those created at SDK startup. This important for when Tracers become configurable and it would be really confusing to deal with two sets of tracers when going to configure how one works.

@tsloughter
Copy link
Member Author

Note that this shouldn't effect any code since they should all be using the macros that hide it. And I think they all do now.

@tsloughter tsloughter merged commit 20c89cf into open-telemetry:main Dec 13, 2021
@tsloughter tsloughter deleted the single-tracer-registry branch December 13, 2021 13:04
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.

2 participants