Conversation
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (25.53%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #2602 +/- ##
============================================
- Coverage 75.45% 75.31% -0.14%
- Complexity 2590 2594 +4
============================================
Files 242 243 +1
Lines 27180 27167 -13
Branches 976 976
============================================
- Hits 20508 20462 -46
- Misses 6152 6185 +33
Partials 520 520
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2024-03-29 15:07:08 Comparing candidate commit 4aac38f in PR branch Found 5 performance improvements and 8 performance regressions! Performance is the same for 168 metrics, 1 unstable metrics. scenario:BM_TeaSapiSpindown
scenario:ComposerTelemetryBench/benchTelemetryParsing-opcache
scenario:MessagePackSerializationBench/benchMessagePackSerialization
scenario:PDOBench/benchPDOBaseline-opcache
scenario:PDOBench/benchPDOOverhead-opcache
scenario:PDOBench/benchPDOOverheadWithDBM-opcache
scenario:SamplingRuleMatchingBench/benchRegexMatching1
scenario:SamplingRuleMatchingBench/benchRegexMatching3
scenario:SymfonyBench/benchSymfonyBaseline
scenario:SymfonyBench/benchSymfonyBaseline-opcache
scenario:SymfonyBench/benchSymfonyOverhead
scenario:SymfonyBench/benchSymfonyOverhead-opcache
|
Signed-off-by: Bob Weinand <[email protected]>
| }, function (HookData $hook) { | ||
| \DDTrace\remove_hook($hook->data); | ||
| }); |
There was a problem hiding this comment.
I'm wondering out loud
I understand that we want the hook on the callback to be set only once, the frankenphp_handle_request being called on each iteration of the main loop.
However, I ponder whether it is better to:
- Install and remove the hook on the callback after the request is dispatched, hence having this overhead for basically all requests (although this happens outside a request)
- This is what's currently done
OR
- Don't remove the callback hook; instead, only install it if it isn't already set. I guess we could check this through a WeakMap on the Closure. This way, I think we could also use
hook_functiononfrankenphp_handle_requestand not remove the hook, which I believe to be less performance-hungry...
However, it is also possible that this change would be so negligible that it isn't meaningful...
Have you considered this way?
There was a problem hiding this comment.
Changed it. I don't think it makes a big difference, but anyway, why not :-)
|
|
||
| consume_distributed_tracing_headers(null); | ||
| }, function (HookData $hook) { | ||
| var_dump($hook->span()); |
Signed-off-by: Bob Weinand <[email protected]>
3 participants
Description
Builds the frankenphp SAPI in docker and creates a span for requests received on the handler.
The created span is pretty lightweight, because frankenphp (unlike roadrunner or Swoole) properly populates the superglobals, so we can rely on the internal default handling.
Reviewer checklist