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

Tracer references tests #759

Merged
merged 3 commits into from
Jul 11, 2022
Merged

Tracer references tests #759

merged 3 commits into from
Jul 11, 2022

Conversation

brettmc
Copy link
Collaborator

@brettmc brettmc commented Jul 11, 2022

a recent bug in examples/traces/demo highlighted that if there is no reference to a tracer provider, then the shared state will shutdown, even if that shared state is being used by active tracers. Adding a phpdoc comment explaining this behavior, and some tests to demonstrate it

a recent bug highlighted that if there is no reference to a tracer provider, then the shared state will shutdown, even if that shared state is being used by active tracers. Adding a phpdoc comment explaining this behavior, and some tests to demonstrate it
@codecov
Copy link

codecov bot commented Jul 11, 2022

Codecov Report

Merging #759 (86f996a) into main (305c1c7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #759   +/-   ##
=========================================
  Coverage     82.86%   82.86%           
  Complexity     1295     1295           
=========================================
  Files           150      150           
  Lines          3192     3192           
=========================================
  Hits           2645     2645           
  Misses          547      547           
Flag Coverage Δ
7.4 82.87% <ø> (ø)
8.0 82.92% <ø> (ø)
8.1 82.92% <ø> (ø)

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

Impacted Files Coverage Δ
src/SDK/Trace/TracerProvider.php 65.00% <ø> (ø)

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 305c1c7...86f996a. Read the comment docs.

@brettmc
Copy link
Collaborator Author

brettmc commented Jul 11, 2022

This documents and adds tests for the current behavior, but I did find the current behavior surprising. I think it's a fairly common practise to avoid creating intermediate objects, but in this case it has a side-effect.
Another option might be that TracerProvider::shutdown is not responsible for destroying shared state, but instead drops its own reference to shared state - thus, if the shared state is in use by a tracer, it can continue to be used until all tracers are removed. That may have been the original behavior prior to #716
Nevay's initial thoughts were:

we can postpone the shutdown until all tracers are out of scope by storing the tracer provider in every tracer, not sure what is better (I'm slightly in favor of keeping the current behavior and just documenting it; mainly because explicit shutdowns are no longer possible if the tracer provider goes out of scope)

The spec doesn't say that TracerProvider::shutdown should affect tracers, only that it should provide NoopTracers after being shut down...

Another argument for changing the current behavior is that if I use DI to create a tracer (in my case, php-di but I don't think that matters), then the reference to the provider can be lost when the tracer has been created (since only the resultant object is stored in DI container). That's going to trip people up in the future, I think.

@Nevay
Copy link
Contributor

Nevay commented Jul 11, 2022

The spec doesn't say that TracerProvider::shutdown should affect tracers, only that it should provide NoopTracers after being shut down...

Tracers are effectively shut down due to shutting down SpanProcessor & co.

TracerProvider shutdown

Shutdown MUST be implemented at least by invoking Shutdown within all internal processors.

SpanProcessor shutdown

Shutdown SHOULD be called only once for each SpanProcessor instance. After the call to Shutdown, subsequent calls to OnStart, OnEnd, or ForceFlush are not allowed. SDKs SHOULD ignore these calls gracefully, if possible.


I did find the current behavior surprising. I think it's a fairly common practise to avoid creating intermediate objects, but in this case it has a side-effect.

It depends on whether we want users to rely on the implicit shutdown behavior and expect them to not call ::shutdown() at all; or treat it only as a fallback to try to export traces if something unexpected happens (exception / process timeout) and expect users to keep a reference to the TracerProvider and call ::shutdown() explicitely under normal circumstances.

thus, if the shared state is in use by a tracer, it can continue to be used until all tracers are removed.

Moving shutdown handling to shared state might be the best option for now.

@bobstrecansky bobstrecansky merged commit 6a14cf9 into open-telemetry:main Jul 11, 2022
@bobstrecansky
Copy link
Collaborator

I merged this because I figured moving shutdown handling to shared state could be handled in a subsequent PR.

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.

None yet

3 participants