Skip to content
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

Merged
merged 8 commits into from
Apr 15, 2024
Merged

feat: Add CakePHP 3+ Support #2618

merged 8 commits into from
Apr 15, 2024

Conversation

PROFeNoM
Copy link
Contributor

@PROFeNoM PROFeNoM commented Apr 8, 2024

Description

Add support for CakePHP 3+ by replacing the hook arguments with the new CakePHP v3+ classes.

Documented in DataDog/documentation#22569

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@PROFeNoM PROFeNoM self-assigned this Apr 8, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2024

Codecov Report

Merging #2618 (dcd2715) into master (e721e34) will decrease coverage by 10.81%.
The diff coverage is 1.19%.

Additional details and impacted files

Impacted file tree graph

@@              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     
Flag Coverage Δ
appsec-extension ?
tracer-extension 78.55% <100.00%> (+<0.01%) ⬆️
tracer-php 50.43% <0.00%> (-29.73%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
ext/integrations/integrations.c 91.11% <100.00%> (+0.09%) ⬆️
...DTrace/Integrations/CakePHP/CakePHPIntegration.php 0.00% <0.00%> (ø)
...tegrations/CakePHP/V2/CakePHPIntegrationLoader.php 0.00% <0.00%> (ø)
...tegrations/CakePHP/V3/CakePHPIntegrationLoader.php 0.00% <0.00%> (ø)

... and 44 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 data
Powered by Codecov. Last update e721e34...dcd2715. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented Apr 8, 2024

Benchmarks

Benchmark execution time: 2024-04-10 09:13:54

Comparing candidate commit 1f3d417 in PR branch alex/feat/cakephp-310 with baseline commit c067f72 in branch master.

Found 1 performance improvements and 3 performance regressions! Performance is the same for 178 metrics, 0 unstable metrics.

scenario:ComposerTelemetryBench/benchTelemetryParsing

  • 🟥 execution_time [+1.953µs; +4.647µs] or [+3.500%; +8.328%]

scenario:LogsInjectionBench/benchLogsInfoInjection-opcache

  • 🟥 execution_time [+207.329ns; +484.271ns] or [+2.279%; +5.324%]

scenario:PDOBench/benchPDOOverheadWithDBM-opcache

  • 🟩 mem_peak [-111.585KB; -111.582KB] or [-4.708%; -4.708%]

scenario:TraceSerializationBench/benchSerializeTrace

  • 🟥 execution_time [+12.967µs; +16.433µs] or [+7.200%; +9.124%]

Comment on lines 36 to 40
/*
if (!defined('CAKE_CORE_INCLUDE_PATH')) {
return self::NOT_AVAILABLE;
}
*/
Copy link
Collaborator

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)?

@PROFeNoM PROFeNoM changed the title feat: Add CakePHP 3 Support feat: Add CakePHP 3+ Support Apr 9, 2024
@PROFeNoM PROFeNoM marked this pull request as ready for review April 9, 2024 07:48
@PROFeNoM PROFeNoM requested a review from a team as a code owner April 9, 2024 07:48
},
]);

\DDTrace\trace_method('Cake\Http\Response', 'getStatusCode', [
Copy link
Collaborator

@bwoebi bwoebi Apr 9, 2024

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.

Copy link
Contributor Author

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;
Copy link
Collaborator

@bwoebi bwoebi Apr 9, 2024

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)

Copy link
Collaborator

@bwoebi bwoebi left a 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 :-)

@PROFeNoM PROFeNoM merged commit bbd3a9c into master Apr 15, 2024
456 of 461 checks passed
@PROFeNoM PROFeNoM deleted the alex/feat/cakephp-310 branch April 15, 2024 11:13
@github-actions github-actions bot added this to the 0.99.0 milestone Apr 15, 2024
@bwoebi bwoebi modified the milestones: 0.99.0, 1.0.0beta1 Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants