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

Resource as an argument to starting the tracer provider #569

Merged
merged 8 commits into from
Apr 5, 2023

Conversation

tsloughter
Copy link
Member

Also a commit to remove the setting of the global tracer to noop when the SDK stops. I think this makes it safer to not potentially drastically harm performance for users on shutdown. The hope would be it is the last thing done but since this can't be guaranteed and technically could be stopped at anytime, so not having a global GC happen seems best.

Instead of setting the global tracer to noop on shutdown of the SDK
we rely on the fact the API already handles the SDK being gone.

Not setting the tracer to noop saves a global GC from the persistent
term update that could negatively impact users.
@codecov
Copy link

codecov bot commented Apr 2, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (a203823) 38.13% compared to head (b2da332) 38.17%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #569      +/-   ##
==========================================
+ Coverage   38.13%   38.17%   +0.04%     
==========================================
  Files          61       61              
  Lines        3598     3599       +1     
==========================================
+ Hits         1372     1374       +2     
+ Misses       2226     2225       -1     
Flag Coverage Δ
api 67.91% <100.00%> (ø)
elixir 18.29% <0.00%> (ø)
erlang 38.16% <100.00%> (+0.04%) ⬆️
exporter 8.11% <ø> (ø)
sdk 78.29% <100.00%> (+0.13%) ⬆️
zipkin 54.16% <ø> (ø)

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

Impacted Files Coverage Δ
apps/opentelemetry_api/src/opentelemetry.erl 80.61% <ø> (ø)
apps/opentelemetry/src/opentelemetry_app.erl 100.00% <100.00%> (ø)
...pps/opentelemetry/src/otel_tracer_provider_sup.erl 100.00% <100.00%> (ø)
apps/opentelemetry/src/otel_tracer_server.erl 65.30% <100.00%> (-0.70%) ⬇️
apps/opentelemetry/src/otel_tracer_server_sup.erl 100.00% <100.00%> (ø)
...pps/opentelemetry_api/src/otel_tracer_provider.erl 68.42% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tsloughter tsloughter force-pushed the terminate-noop-default branch from 9301557 to 62f6c94 Compare April 2, 2023 22:18
@tsloughter
Copy link
Member Author

Eh.. this may be wrong, it requires making the resource type available in the API. Rethinking.

@tsloughter tsloughter force-pushed the terminate-noop-default branch from fd3494d to da9f663 Compare April 3, 2023 22:42
this better follows the spec
@tsloughter tsloughter force-pushed the terminate-noop-default branch from da9f663 to 6d71b71 Compare April 3, 2023 22:47
@tsloughter tsloughter force-pushed the terminate-noop-default branch from 7ed076c to 2300d09 Compare April 4, 2023 23:55
@tsloughter tsloughter force-pushed the terminate-noop-default branch from 41844ab to b2da332 Compare April 5, 2023 12:59
@tsloughter tsloughter merged commit 491e553 into open-telemetry:main Apr 5, 2023
@tsloughter tsloughter deleted the terminate-noop-default branch April 5, 2023 21:48
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