-
Notifications
You must be signed in to change notification settings - Fork 160
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
feat: Add CakePHP 3+ Support #2618
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2618 +/- ##
=============================================
- Coverage 77.64% 66.83% -10.81%
- Complexity 2226 2240 +14
=============================================
Files 226 202 -24
Lines 25942 22036 -3906
Branches 986 0 -986
=============================================
- Hits 20142 14728 -5414
- Misses 5274 7308 +2034
+ Partials 526 0 -526
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 44 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
BenchmarksBenchmark execution time: 2024-04-10 09:13:54 Comparing candidate commit 1f3d417 in PR branch Found 1 performance improvements and 3 performance regressions! Performance is the same for 178 metrics, 0 unstable metrics. scenario:ComposerTelemetryBench/benchTelemetryParsing
scenario:LogsInjectionBench/benchLogsInfoInjection-opcache
scenario:PDOBench/benchPDOOverheadWithDBM-opcache
scenario:TraceSerializationBench/benchSerializeTrace
|
/* | ||
if (!defined('CAKE_CORE_INCLUDE_PATH')) { | ||
return self::NOT_AVAILABLE; | ||
} | ||
*/ |
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 suppose you will want to continue checking for these for CakePHP V2 (i.e. if the class_exists check fails)?
}, | ||
]); | ||
|
||
\DDTrace\trace_method('Cake\Http\Response', 'getStatusCode', [ |
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 suppose you copied from V2, but shouldn't we just use proper hook_method for these? I mean return false
just unconditionally drops the span...
I think we can improve that one in all instances here. Then this instrument_when_limited thing also isn't necessary.
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.
Absolutely :) Copy-pasted as you said and didn't pay attention to this; I'll change these
{ | ||
public function load($integration) | ||
{ | ||
$integration->rootSpan = null; |
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 move away from caching rootSpan and rather refetch \DDTrace\root_span(), as more and more frameworks are also working under Swoole (where the root span isn't global, but may change with the executing fiber).
For CakePHP v2 this didn't matter, but modern versions of this don't have that much global state anymore (e.g. https://github.com/rochamarcelo/cakephp-api-swoole-sample)
1f3d417
to
dcd2715
Compare
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.
Looks good to me :-)
Description
Add support for CakePHP 3+ by replacing the hook arguments with the new CakePHP v3+ classes.
Documented in DataDog/documentation#22569
Reviewer checklist