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

separate application tracer registration from user named tracers #287

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

tsloughter
Copy link
Member

Fixes #279

Because persistent_term:put slows down as the number of terms
grows a project with thousands of modules can cause startup to
timeout as it sets a term for every module in it. This patch
replaces that term for each module with a single term containing
a map of module => tracer.

Users must be able to register named tracers at runtime and we
want to not trigger a global GC by updating the tracer map
used for module to tracer mapping. So any user registered tracer
will have its own persistent_term.

The macro for looking up a tracer used in macros like ?with_span
now looks up the tracer by looking up the module in the
"application tracers" (the persistent term containing the map).
While a user looking up a named tracer will do so with the function
opentelemetry:get_tracer/1 which looks directly for a term with
that name.

This means a user could register a tracer with the same name as a
module. Whether this is good or bad or needs a check to guard
against is not covered by this PR.

@tsloughter tsloughter marked this pull request as ready for review October 6, 2021 22:09
@tsloughter tsloughter requested a review from a team October 6, 2021 22:09
@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #287 (6201a1a) into main (51ce24e) will increase coverage by 0.04%.
The diff coverage is 83.33%.

❗ Current head 6201a1a differs from pull request most recent head 64cbc0a. Consider uploading reports for the commit 64cbc0a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
+ Coverage   37.28%   37.32%   +0.04%     
==========================================
  Files          47       46       -1     
  Lines        3251     3255       +4     
==========================================
+ Hits         1212     1215       +3     
- Misses       2039     2040       +1     
Flag Coverage Δ
api 65.46% <81.81%> (+0.09%) ⬆️
elixir 14.28% <6.06%> (-0.43%) ⬇️
erlang 37.33% <83.33%> (+0.04%) ⬆️
exporter 19.75% <ø> (ø)
sdk 77.15% <100.00%> (+0.22%) ⬆️

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

Impacted Files Coverage Δ
...pps/opentelemetry_api/src/otel_tracer_provider.erl 69.56% <75.00%> (-2.44%) ⬇️
apps/opentelemetry_api/src/opentelemetry.erl 76.25% <82.75%> (-1.17%) ⬇️
apps/opentelemetry/src/opentelemetry_app.erl 100.00% <100.00%> (ø)
apps/opentelemetry/src/otel_tracer_server.erl 82.75% <100.00%> (+0.94%) ⬆️
apps/opentelemetry/src/otel_resource_detector.erl 91.42% <0.00%> (-1.43%) ⬇️

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 51ce24e...64cbc0a. Read the comment docs.

register_tracer/2,
register_application_tracer/1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yup, thanks!

Wonder if mix xref should be catching that.

@tsloughter tsloughter force-pushed the single-tracer-term branch 2 times, most recently from a24a60d to 0ea8959 Compare October 8, 2021 12:18
@tsloughter
Copy link
Member Author

As I was updating the changelog just now I had a thought.

In current and previous versions a user registering a tracer mod_a would result in tracer macros used in a module mod_a using this new tracer instead of the tracer named for the application app_a that the mod_a module is in.

This made no sense since mod_a wasn't even the name of the tracer.

In this PR, as it stands now, a user registering tracer mod_a would not effect macro use in the module mod_a.

However, neither would a user registering a tracer named app_a, when it probably should.

A solution would be to still register all tracers as persistent_terms and the large map would point a module name to a tracer name (application name) which is then looked up. This would greatly shrink what is stored in the map as right now it is a lot of duplication, but would require yet another lookup, semi-psuedo code:

Map = persistent_term:get(module_tracer_map)
TracerName = maps:get(Module, Map)
persistent_term:get(TracerName)

That map looking like #{module() => atom()} instead of #{module => #tracer{}} where the #tracer{} record is the same for every module in the same application.

I think those duplicate tracer records would be optimized away if the map was constructed like, #{a => Tracer, b => Tracer} but not sure what happens in the case of how we build the map and it being stored in a persistent term.

So should first verify that the tracer record is indeed duplicated.

@tsloughter tsloughter force-pushed the single-tracer-term branch 4 times, most recently from a1e8321 to eaa2a54 Compare October 8, 2021 16:43
Because `persistent_term:put` slows down as the number of terms
grows a project with thousands of modules can cause startup to
timeout as it sets a term for every module in it. This patch
replaces that term for each module with a single term containing
a map of `module => tracer`.

Users must be able to register named tracers at runtime and we
want to not trigger a global GC by updating the tracer map
used for module to tracer mapping. So any user registered tracer
will have its own `persistent_term`.

The macro for looking up a tracer used in macros like `?with_span`
now looks up the tracer by looking up the module in the
"application tracers" (the persistent term containing the map).
While a user looking up a named tracer will do so with the function
`opentelemetry:get_tracer/1` which looks directly for a term with
that name.

This means a user could register a tracer with the same name as a
module. Whether this is good or bad or needs a check to guard
against is not covered by this PR.
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.

Use single persistent term to map modules to tracers
3 participants