opentelemetry-php-contrib
opentelemetry-php-contrib copied to clipboard
open-telemetry
Fix boundaries for Symfony HttpKernel auto instrumentation
Fixes https://github.com/open-telemetry/opentelemetry-php/issues/1443
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.
![welcome[bot] avatar](https://avatars.githubusercontent.com/in/4141?v=4 "Published by a pub.dev verified publisher"){kind=small}
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: dciprian-petrisor / name: Petrisor Ciprian Daniel (24e30e19bc19e8c6393bcc42da2ca7676f463f71)
![linux-foundation-easycla[bot] avatar](https://avatars.githubusercontent.com/in/17893?v=4 "Published by a pub.dev verified publisher"){kind=small}
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 82.81%. Comparing base (3d6f839) to head (24e30e1).
:warning: Report is 5 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #317 +/- ##
============================================
- Coverage 82.85% 82.81% -0.04%
+ Complexity 2353 2309 -44
============================================
Files 164 159 -5
Lines 9563 9256 -307
============================================
- Hits 7923 7665 -258
+ Misses 1640 1591 -49
Flags with carried forward coverage won't be shown. Click here to find out more. see 5 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 3d6f839...24e30e1. Read the comment docs.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
![stale[bot] avatar](https://avatars.githubusercontent.com/in/1724?v=4 "Published by a pub.dev verified publisher"){kind=small}
Just running into the exact same issue in a project I am working on. Is this on the horizon to be merged any time soon?
Just running into the exact same issue in a project I am working on. Is this on the horizon to be merged any time soon?
No. I'm not sure of the current state. It at least needs to be rebased given its age, and there's some discussion that didn't conclude (and I don't know enough Symfony to make a call myself) @dciprian-petrisor ?
Just running into the exact same issue in a project I am working on. Is this on the horizon to be merged any time soon?
No. I'm not sure of the current state. It at least needs to be rebased given its age, and there's some discussion that didn't conclude (and I don't know enough Symfony to make a call myself) @dciprian-petrisor ?
Hi, I'll look into this over the weekend and if you all agree we could merge it.
I've rebased this PR.
After a brief look with fresh eyes, given it's been some time since this PR had activity, here are my conclusions:
- The implementation in this PR covers the 'normal' execution of any Symfony HTTP Request, minus the situation where any of the following calls in HttpKernelRunner would fail miserably:
fastcgi_finish_request();
or
litespeed_finish_request();
or
Response::closeOutputBuffers(0, true);
flush();
All three of these failing indicate a severe problem and I have personally never encountered failures of them. These calls failing would indicate issues with FASTCGI/PHP internals or the SAPI.
- In any other situation, one of the following two outcomes can happen:
- the
posthook ofhandleruns whenever an exception occurs/is thrown while processing the Symfony request. otherwise, aResponseobject is returned fromhandle. - the
terminatecall is always made whenhandledoes not throw.
Both of these will finalize the trace.
In summary, I believe this implementation is good as-is. Regarding the documentation, if you can give me a few hints as to where to document it, I would gladly do it.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
In there any chance of getting some progress on this? Realise @dciprian-petrisor rebased this a couple of months back.
This issue currently causes quite a bit of pain when using the Symfony autoinstrumentation libaries.
Very sorry, I didn't see the update. @dciprian-petrisor are you able to (again) resolve conflicts?
Sorry to chase again, any idea when we might see this merged / released @brettmc?