Add exec integration #2361
Add exec integration #2361
Conversation
cataphract
force-pushed
the
glopes/exec-integration
branch
2 times, most recently
from
d56c4bc
to
d1c3538
Compare
ext/integrations/exec_integration.c
Outdated
| // clang-format off | ||
| ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(register_stream, 0, 2, _IS_BOOL, 0) | ||
| ZEND_ARG_TYPE_INFO(0, stream, IS_RESOURCE, 0) | ||
| ZEND_ARG_OBJ_INFO(0, span, DDTrace\\SpanData, 0) | ||
| ZEND_END_ARG_INFO() | ||
| ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(proc_assoc_span, 0, 2, _IS_BOOL, 1) | ||
| ZEND_ARG_TYPE_INFO(0, proc_res, IS_RESOURCE, 0) | ||
| ZEND_ARG_OBJ_INFO(0, span, DDTrace\\SpanData, 0) | ||
| ZEND_END_ARG_INFO() | ||
| ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(proc_get_span, 0, 1, DDTrace\\SpanData, 0) | ||
| ZEND_ARG_TYPE_INFO(0, proc_res, IS_RESOURCE, 0) | ||
| ZEND_END_ARG_INFO() | ||
| ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(proc_get_pid, 0, 1, IS_LONG, 1) | ||
| ZEND_ARG_TYPE_INFO(0, proc_res, IS_RESOURCE, 0) | ||
| ZEND_END_ARG_INFO() | ||
| ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(test_rshutdown, 0, 0, IS_NULL, 1) | ||
| ZEND_END_ARG_INFO() | ||
|
|
||
| static const zend_function_entry functions[] = { | ||
| ZEND_RAW_FENTRY(NS "register_stream", PHP_FN(DDTrace_integrations_exec_register_stream), register_stream, 0) | ||
| ZEND_RAW_FENTRY(NS "proc_assoc_span", PHP_FN(DDTrace_integrations_exec_proc_assoc_span), proc_assoc_span, 0) | ||
| ZEND_RAW_FENTRY(NS "proc_get_span", PHP_FN(DDTrace_integrations_exec_proc_get_span), proc_get_span, 0) | ||
| ZEND_RAW_FENTRY(NS "proc_get_pid", PHP_FN(DDTrace_integrations_exec_proc_get_pid), proc_get_pid, 0) | ||
| ZEND_RAW_FENTRY(NS "test_rshutdown", PHP_FN(DDTrace_integrations_exec_test_rshutdown), test_rshutdown, 0) | ||
| PHP_FE_END | ||
| }; |
There was a problem hiding this comment.
Can you please add the definitions to ddtrace.stub.php?
A pure testing function like exec_test_reshutdown and proc_get_pid should be proably hidden away in dd_trace_internal_fn - we're trying to be a bit tidy regarding testing functions :-)
There was a problem hiding this comment.
@cataphract You moved it to the stub file, but did not regenerate the arginfo: in php-src repo, there's a build/gen_stub.php script, which needs to be run.
There was a problem hiding this comment.
I put them in a different stub file.
As to moving them do dd_trace_internal_fn I don't think it's a good idea. I'd either have to duplicate logic from integrations_exec (fetching the resource type and a conditional typedef) or to export global functions in integrations_exec.c. Either way, it's a solution with less code cohesion. Also, proc_get_pid doesn't fit the signature of dd_trace_internal_fn, which returns a bool. The only disadvantage of the current solution is that are more PHP functions registered, but they are in a namespace that only has internal functions anyway.
cataphract
force-pushed
the
glopes/exec-integration
branch
7 times, most recently
from
8c99e37
to
ebbf556
Compare
cataphract
force-pushed
the
glopes/exec-integration
branch
3 times, most recently
from
ca57039
to
154e4a9
Compare
|
Can you add all internal functions to |
cataphract
force-pushed
the
glopes/exec-integration
branch
4 times, most recently
from
dece386
to
2529033
Compare
There was a problem hiding this comment.
lgtm, I would like to see some feedback from @estringana and @PROFeNoM as well though, but as far as I can tell everything looks correct and following the specification.
| } | ||
|
|
||
| if ($hook->returned === -1 && isset($span->meta[Tag::EXEC_EXIT_CODE])) { | ||
| $hook->overrideReturnValue($span->meta[Tag::EXEC_EXIT_CODE]); |
There was a problem hiding this comment.
This clarifies things, perhaps you should document a bit better the proc_open implementation.
cataphract
force-pushed
the
glopes/exec-integration
branch
from
2529033
to
f4edf58
Compare
|
|
||
| #define NS "DDTrace\\Integrations\\Exec\\" | ||
|
|
||
| #if PHP_VERSION_ID <= 80000 |
Description
Create spans for exec functions in PHP.
Related Jiras: APPSEC-11906, APPSEC-11909, APPSEC-11910, APPSEC-11911, APPSEC-11912, APPSEC-12116
Readiness checklist
Reviewer checklist