Remove automatic ::shutdown(), add ShutdownHandler::register()#760
Conversation
Codecov Report
@@ Coverage Diff @@
## main #760 +/- ##
============================================
- Coverage 79.59% 78.43% -1.17%
- Complexity 1295 1331 +36
============================================
Files 150 153 +3
Lines 3195 3278 +83
============================================
+ Hits 2543 2571 +28
- Misses 652 707 +55
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This approach won't work for metrics; the metrics API allows registration of permanent callbacks. Asynchronous Instrument API:
I see three options:
We should also consider that |
I'm inclined to think option 3: if you want to shut down a tracer provider, you must keep a reference to it and explicitly call |
8bc39a5 to
a98fcba
Compare
Nevay
left a comment
Nevay
left a comment
There was a problem hiding this comment.
Does that resurrect any tracer provider leaks that you fixed up in the earlier PR?
No, __destruct() could simply call ::unregisterShutdownFunction() instead of ::shutdown().
Converting this to draft for now; I've decided to create a POC that allows users to opt-in instead of enabling it by default.
ShutdownHandler::register($tracerProvider->shutdown(...)); // Closure::fromCallable([$provider, 'shutdown']) if php < 8.1| /** | ||
| * @see https://github.com/amphp/amp/blob/f682341c856b1f688026f787bef4f77eaa5c7970/src/functions.php#L140-L191 | ||
| */ | ||
| private static function weaken(Closure $closure, ?object &$target = null): Closure |
There was a problem hiding this comment.
This is also very useful for the metrics API; we can add a $weak parameter to asynchronous instruments ::observe() method that would allow users to provide normal callbacks with bound $this instead of forcing them to take care of weakening the $this reference themselves.
6a41de0 to
14e6909
Compare
14e6909 to
7d003ee
Compare
Nevay
changed the title
::shutdown() until tracers/not ended spans are out of scope::shutdown(), add ShutdownHandler::register()
|
@Nevay let us know if it's ok to merge this now. |
|
Yes, this can be merged. |
Since changes in open-telemetry/opentelemetry-php#760, the original Kernel Listener example no longer works... Following those changes, the stored Tracer instance gets deleted before the application terminates so mainSpan can't be ended and none of the collected trace info gets sent. By refactoring to use TracerProvider instead, mainSpan's Tracer remains valid during KernelEvents::TERMINATE so the trace session can be sent successfully.
Since changes in open-telemetry/opentelemetry-php#760, the original Kernel Listener example no longer works... Following those changes, the stored Tracer instance gets deleted before the application terminates so mainSpan can't be ended and none of the collected trace info gets sent. By refactoring to use TracerProvider instead, mainSpan's Tracer remains valid during KernelEvents::TERMINATE so the trace session can be sent successfully.
Since changes in open-telemetry/opentelemetry-php#760, the original Kernel Listener example no longer works... Following those changes, the stored Tracer instance gets deleted before the application terminates so mainSpan can't be ended and none of the collected trace info gets sent. By refactoring to use TracerProvider instead, mainSpan's Tracer remains valid during KernelEvents::TERMINATE so the trace session can be sent successfully.
Since changes in open-telemetry/opentelemetry-php#760, the original Kernel Listener example no longer works... Following those changes, the stored Tracer instance gets deleted before the application terminates so mainSpan can't be ended and none of the collected trace info gets sent. By refactoring to use TracerProvider instead, mainSpan's Tracer remains valid during KernelEvents::TERMINATE so the trace session can be sent successfully.
3 participants
See #759, removes automatic shutdown handling. Users should keep a reference to the
TracerProviderand call::shutdown()explicitely.ShutdownHandler::register([$tracerProvider, 'shutdown'])can be used to protect against process timeout / exit, but also requires users to keep a reference to theTracerProvider.The tracer provider should be in scope anyways to be globally accessible in the current execution context (somewhat depends on how instrumentation registration is solved?).