Skip to content

Add DDTrace namespace for internal functions#930

Merged
SammyK merged 3 commits intomasterfrom
sammyk/namespace
Jun 22, 2020
Merged

Add DDTrace namespace for internal functions#930
SammyK merged 3 commits intomasterfrom
sammyk/namespace

Conversation

@SammyK
Copy link
Copy Markdown
Contributor

@SammyK SammyK commented Jun 19, 2020 •
edited by morrisonlevi
Loading

Description

This PR adds the DDTrace namespace to internal functions so that going forward, all new API's can be added under this namespace. This change only applies to two existing functions:

  • dd_trace_function -> DDTrace\trace_function
  • dd_trace_method -> DDTrace\trace_method

The old functions dd_trace_function & dd_trace_method remain as aliases to their namespaced counterparts.

Readiness checklist

  • (only for Members) Changelog has been added to the appropriate release draft. Create one if necessary.
  • Tests added for this feature/bug.

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.
  • Changelog has been added to the appropriate release draft. For community contributors the reviewer is in charge of this task.

@SammyK SammyK added 🏆 enhancement A new feature or improvement c-extension Apply this label to issues and prs related to the C-extension labels Jun 19, 2020
@SammyK SammyK added this to the 0.47.0 milestone Jun 19, 2020
@SammyK SammyK force-pushed the sammyk/namespace branch from 90bf5a1 to 03c8444 Compare June 19, 2020 22:05
SammyK added 3 commits June 19, 2020 15:14
@SammyK
Add DDTrace namespace for internal functions
143bfad 
This only changes two functions:

* dd_trace_function -> DDTrace\trace_function
* dd_trace_method -> DDTrace\trace_method

dd_trace_function & dd_trace_method remain as aliases
@SammyK
Update tracing functions to DDTrace\trace_function & DDTrace\trace_me…
8ac6800 
…thod
@SammyK
Fix linting
4fac037
@SammyK SammyK force-pushed the sammyk/namespace branch from 03c8444 to 4fac037 Compare June 19, 2020 22:14
labbati
labbati approved these changes Jun 22, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SammyK. On question about the $s and $a arguments when unused and a request to confirm that DDTrace\trace_method (without leading \) in phpt files is intentional.

Comment thread src/DDTrace/Integrations/Slim/SlimSandboxedIntegration.php

// Hook into the router to extract the proper route name
\dd_trace_method('Slim\Router', 'lookupRoute', function (SpanData $span, $args, $return) use ($rootSpan) {
\DDTrace\trace_method('Slim\Router', 'lookupRoute', function (SpanData $s, $a, $return) use ($rootSpan) {
Copy link
Copy Markdown
Member

@labbati labbati Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just asking to adopt the same approach in similar scenarios. Do we want to use $s and $a meaning 'not used'? Something like the _ of other languages?

Copy link
Copy Markdown
Contributor Author

@SammyK SammyK Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea! I changed these just to get the linter to pass, but having a consistent "not used" convention would be good to implement across the board. Are we good with the first initial of each param: $s, $a, $r? Or something else?

Comment thread tests/ext/sandbox-prehook/access_modifier_method_access_hook.phpt
Comment thread tests/ext/sandbox-prehook/access_modifier_method_access_hook.phpt
Comment thread tests/ext/sandbox-prehook/access_modifier_property_access_hook.phpt
@SammyK SammyK merged commit 5edff86 into master Jun 22, 2020
@SammyK SammyK deleted the sammyk/namespace branch June 22, 2020 14:36
@labbati
Copy link
Copy Markdown
Member

labbati commented Jun 22, 2020

Thanks for the DDTrace in phpt files explanation @SammyK , resolved all the comments now, good to merge for me.

@morrisonlevi morrisonlevi mentioned this pull request Jul 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@labbati labbati labbati approved these changes

@morrisonlevi morrisonlevi morrisonlevi approved these changes

Assignees

No one assigned

Labels

c-extension Apply this label to issues and prs related to the C-extension 🏆 enhancement A new feature or improvement

Projects

None yet

Milestone

0.47.0

Development

Successfully merging this pull request may close these issues.

3 participants

@SammyK @labbati @morrisonlevi