-
Notifications
You must be signed in to change notification settings - Fork 149
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
fix: Background Sender reset on Swoole Fork #2645
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2645 +/- ##
============================================
- Coverage 79.35% 77.64% -1.72%
- Complexity 2198 2205 +7
============================================
Files 199 225 +26
Lines 21959 25933 +3974
Branches 0 986 +986
============================================
+ Hits 17426 20136 +2710
- Misses 4533 5271 +738
- Partials 0 526 +526
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 32 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
1b7a816
to
a1f8cf9
Compare
BenchmarksBenchmark execution time: 2024-04-30 15:48:53 Comparing candidate commit 02d0ceb in PR branch Found 1 performance improvements and 1 performance regressions! Performance is the same for 176 metrics, 0 unstable metrics. scenario:ContextPropagationBench/benchExtractTraceContext128Bit-opcache
scenario:TraceSerializationBench/benchSerializeTrace
|
…o_compile=false" under CI env
6382bd2
to
3af510f
Compare
ext/ddtrace.stub.php
Outdated
/** | ||
* @internal | ||
*/ | ||
function dd_handle_fork(): void {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please add no new functions to top-level namespace, but add to DDTrace
or DDTrace\Internal
as appropriate?
ext/ddtrace.c
Outdated
PHP_FUNCTION(dd_handle_fork) { | ||
UNUSED(execute_data); | ||
UNUSED(return_value); | ||
// CHILD PROCESS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it's still todo to deduplicate with the code in handlers_pcntl.c?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you exactly mean by "deduplicate"? Do you mean some refactoring duplicated code (using this function in the pcntl code)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, refactoring the common code out into a single function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good :-)
Description
#2636
Reviewer checklist