Fix boundaries for Symfony HttpKernel auto instrumentation #317
Conversation
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
|
|
dciprian-petrisor
force-pushed
the
fix-symfony-auto-instrumentation-boundaries
branch
from
e78c015
to
19f787e
Compare
| @@ -80,6 +80,30 @@ public static function register(): void | |||
| array $params, | |||
| ?Response $response, | |||
| ?\Throwable $exception | |||
| ): void { | |||
| $scope = Context::storage()->scope(); | |||
| if (null === $scope || null === $exception) { | |||
There was a problem hiding this comment.
So if an exception occurs, this post callback will run, otherwise the terminate one will? If so, we should document this.
There was a problem hiding this comment.
That is correct.
I'm assuming this should be documented in the README.md, right?
There was a problem hiding this comment.
The examples on this page seem to suggest that the handle and terminate can both run.
So it looks like you'd expect 2 spans to be created and then with these changes only close one of them? I'm not so sure that behaviour is correct.
There was a problem hiding this comment.
There appears to be a parameter to configure whether handle should throw here.
So when $catch = false the handle post-hook might have an exception and terminate will not have been reached yet. I think this would be fine as it handles the recording of the exception and detaches the scope.
However, if $catch = false and there is no exception, then terminate() would be called after handle and the terminate post-hook could try to close a scope which does not belong to it.
There was a problem hiding this comment.
-
If
handlethrows an exception which is never caught, thenterminatewill never be reached either, according to theHttpKernelRunnerimplementation.
In this situation, it is correct to close the scope on theposthook ofhandle. -
If
handledoes not throw an exception (or has$catch = true), thenterminatewould run afterwards, closing the scope.
The only problem I see is there is not a 100% guarantee that terminate will always be executed after handle.
For example, something else might go wrong in HttpKernelRunner after handle was called, e.g. fastcgi_finish_request()/litespeed_finish_request() might crash, flush() might crash, etc, leaving the scope still open.
Unsure how much of a real issue this is, in most cases.
Normally this would be solved by hooking into HttpKernelRunner::run with both pre and post hooks, as it covers both handle and terminate, however $request and $response are not available as parameters there, so I am at a loss right now.
The main issue is that the parent span is lost in between handle and terminate, due to handle detaching it, thus producing two different traces for the same request:
- one containing everything from the start of the HTTP request up to the end of
handle - one containing just the dispatch of
TerminateEventdone by$this->kernel->terminateafterwards
the second should be part of the first, as it is still logically part of the same HTTP request lifecycle.
We need either a compromise or another solution entirely.
There was a problem hiding this comment.
Thanks for that @dciprian-petrisor - I had another look with less bleary eyes and I finally caught up to you guys 💪
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #317 +/- ##
============================================
- Coverage 79.60% 78.66% -0.95%
+ Complexity 729 610 -119
============================================
Files 69 60 -9
Lines 2952 2606 -346
============================================
- Hits 2350 2050 -300
+ Misses 602 556 -46
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Fixes open-telemetry/opentelemetry-php#1443