-
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
Adds TextMap propagator #6
Conversation
const TRACE_ID = '1c42b4de015cc315'; | ||
const SPAN_ID = '1c42b4de015cc316'; | ||
|
||
public function testInjectSpanContextIntoCarrier() |
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 add some tests where:
- what happen if the
traceId
is not set - what happen if the
spanId
is not set
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 think you refer to extract
not to inject
.
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.
oh yes sorry 👍
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.
Done.
src/DDTrace/Propagators/TextMap.php
Outdated
continue; | ||
} | ||
|
||
if ($traceId === null && $spanId === 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.
I think it should be an ||
operator like we're doing for Go: https://github.com/DataDog/dd-trace-go/blob/master/opentracing/propagators.go#L116-L118
Mostly a SpanContext
is not valid for extraction if the traceId
and the spanId
(the parent) are not set. In case of distributed tracing they both need to be set, correct?
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.
Totally. I will change it.
Illustrating the issue: // calling begin adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_begin add x8, x8, #0x6b0 blr x8 // start of function execution // reading string arg from execute_data constants mov x8, #0xe400 // #58368 movk x8, #0xac44, lsl #16 movk x8, #0xaaaa, lsl #32 // storing to CV[1] str x8, [x27, #96] mov w8, #0x6 // #6 // storing type of CV[1] str w8, [x27, #104] // nothing happens, my test function is empty :-) // calling end with 2 args adrp x1, 0xaaaaacad4000 add x1, x1, #0x740 mov x0, x27 adrp x8, 0xaaaaacad4000 add x8, x8, #0x7b0 str x8, [x27] adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_end add x8, x8, #0x844 blr x8 As one can see, with the tracing JIT, it just forcibly overrides the second arg, inlining the called function. (Which is also why we leaked our custom $context array in the LogsIntegration). The JIT has no exit points here. Hence, we have no choice but to forcefully disallow inlining the hooked methods where overrideArguments is used. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Illustrating the issue: // calling begin adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_begin add x8, x8, #0x6b0 blr x8 // start of function execution // reading string arg from execute_data constants mov x8, #0xe400 // #58368 movk x8, #0xac44, lsl #16 movk x8, #0xaaaa, lsl #32 // storing to CV[1] str x8, [x27, #96] mov w8, #0x6 // #6 // storing type of CV[1] str w8, [x27, #104] // nothing happens, my test function is empty :-) // calling end with 2 args adrp x1, 0xaaaaacad4000 add x1, x1, #0x740 mov x0, x27 adrp x8, 0xaaaaacad4000 add x8, x8, #0x7b0 str x8, [x27] adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_end add x8, x8, #0x844 blr x8 As one can see, with the tracing JIT, it just forcibly overrides the second arg, inlining the called function. (Which is also why we leaked our custom $context array in the LogsIntegration). The JIT has no exit points here. Hence, we have no choice but to forcefully disallow inlining the hooked methods where overrideArguments is used. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Illustrating the issue: // calling begin adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_begin add x8, x8, #0x6b0 blr x8 // start of function execution // reading string arg from execute_data constants mov x8, #0xe400 // #58368 movk x8, #0xac44, lsl #16 movk x8, #0xaaaa, lsl #32 // storing to CV[1] str x8, [x27, #96] mov w8, #0x6 // #6 // storing type of CV[1] str w8, [x27, #104] // nothing happens, my test function is empty :-) // calling end with 2 args adrp x1, 0xaaaaacad4000 add x1, x1, #0x740 mov x0, x27 adrp x8, 0xaaaaacad4000 add x8, x8, #0x7b0 str x8, [x27] adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_end add x8, x8, #0x844 blr x8 As one can see, with the tracing JIT, it just forcibly overrides the second arg, inlining the called function. (Which is also why we leaked our custom $context array in the LogsIntegration). The JIT has no exit points here. Hence, we have no choice but to forcefully disallow inlining the hooked methods where overrideArguments is used. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Illustrating the issue: // calling begin adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_begin add x8, x8, #0x6b0 blr x8 // start of function execution // reading string arg from execute_data constants mov x8, #0xe400 // #58368 movk x8, #0xac44, lsl #16 movk x8, #0xaaaa, lsl #32 // storing to CV[1] str x8, [x27, #96] mov w8, #0x6 // #6 // storing type of CV[1] str w8, [x27, #104] // nothing happens, my test function is empty :-) // calling end with 2 args adrp x1, 0xaaaaacad4000 add x1, x1, #0x740 mov x0, x27 adrp x8, 0xaaaaacad4000 add x8, x8, #0x7b0 str x8, [x27] adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_end add x8, x8, #0x844 blr x8 As one can see, with the tracing JIT, it just forcibly overrides the second arg, inlining the called function. (Which is also why we leaked our custom $context array in the LogsIntegration). The JIT has no exit points here. Hence, we have no choice but to forcefully disallow inlining the hooked methods where overrideArguments is used. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Illustrating the issue: // calling begin adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_begin add x8, x8, #0x6b0 blr x8 // start of function execution // reading string arg from execute_data constants mov x8, #0xe400 // #58368 movk x8, #0xac44, lsl #16 movk x8, #0xaaaa, lsl #32 // storing to CV[1] str x8, [x27, #96] mov w8, #0x6 // #6 // storing type of CV[1] str w8, [x27, #104] // nothing happens, my test function is empty :-) // calling end with 2 args adrp x1, 0xaaaaacad4000 add x1, x1, #0x740 mov x0, x27 adrp x8, 0xaaaaacad4000 add x8, x8, #0x7b0 str x8, [x27] adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_end add x8, x8, #0x844 blr x8 As one can see, with the tracing JIT, it just forcibly overrides the second arg, inlining the called function. (Which is also why we leaked our custom $context array in the LogsIntegration). The JIT has no exit points here. Hence, we have no choice but to forcefully disallow inlining the hooked methods where overrideArguments is used. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Illustrating the issue: // calling begin adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_begin add x8, x8, #0x6b0 blr x8 // start of function execution // reading string arg from execute_data constants mov x8, #0xe400 // #58368 movk x8, #0xac44, lsl #16 movk x8, #0xaaaa, lsl #32 // storing to CV[1] str x8, [x27, #96] mov w8, #0x6 // #6 // storing type of CV[1] str w8, [x27, #104] // nothing happens, my test function is empty :-) // calling end with 2 args adrp x1, 0xaaaaacad4000 add x1, x1, #0x740 mov x0, x27 adrp x8, 0xaaaaacad4000 add x8, x8, #0x7b0 str x8, [x27] adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_end add x8, x8, #0x844 blr x8 As one can see, with the tracing JIT, it just forcibly overrides the second arg, inlining the called function. (Which is also why we leaked our custom $context array in the LogsIntegration). The JIT has no exit points here. Hence, we have no choice but to forcefully disallow inlining the hooked methods where overrideArguments is used. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Illustrating the issue: // calling begin adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_begin add x8, x8, #0x6b0 blr x8 // start of function execution // reading string arg from execute_data constants mov x8, #0xe400 // #58368 movk x8, #0xac44, lsl #16 movk x8, #0xaaaa, lsl #32 // storing to CV[1] str x8, [x27, #96] mov w8, #0x6 // #6 // storing type of CV[1] str w8, [x27, #104] // nothing happens, my test function is empty :-) // calling end with 2 args adrp x1, 0xaaaaacad4000 add x1, x1, #0x740 mov x0, x27 adrp x8, 0xaaaaacad4000 add x8, x8, #0x7b0 str x8, [x27] adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_end add x8, x8, #0x844 blr x8 As one can see, with the tracing JIT, it just forcibly overrides the second arg, inlining the called function. (Which is also why we leaked our custom $context array in the LogsIntegration). The JIT has no exit points here. Hence, we have no choice but to forcefully disallow inlining the hooked methods where overrideArguments is used. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Illustrating the issue: // calling begin adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_begin add x8, x8, #0x6b0 blr x8 // start of function execution // reading string arg from execute_data constants mov x8, #0xe400 // #58368 movk x8, #0xac44, lsl #16 movk x8, #0xaaaa, lsl #32 // storing to CV[1] str x8, [x27, #96] mov w8, #0x6 // #6 // storing type of CV[1] str w8, [x27, #104] // nothing happens, my test function is empty :-) // calling end with 2 args adrp x1, 0xaaaaacad4000 add x1, x1, #0x740 mov x0, x27 adrp x8, 0xaaaaacad4000 add x8, x8, #0x7b0 str x8, [x27] adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_end add x8, x8, #0x844 blr x8 As one can see, with the tracing JIT, it just forcibly overrides the second arg, inlining the called function. (Which is also why we leaked our custom $context array in the LogsIntegration). The JIT has no exit points here. Hence, we have no choice but to forcefully disallow inlining the hooked methods where overrideArguments is used. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Illustrating the issue: // calling begin adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_begin add x8, x8, #0x6b0 blr x8 // start of function execution // reading string arg from execute_data constants mov x8, #0xe400 // #58368 movk x8, #0xac44, lsl #16 movk x8, #0xaaaa, lsl #32 // storing to CV[1] str x8, [x27, #96] mov w8, #0x6 // #6 // storing type of CV[1] str w8, [x27, #104] // nothing happens, my test function is empty :-) // calling end with 2 args adrp x1, 0xaaaaacad4000 add x1, x1, #0x740 mov x0, x27 adrp x8, 0xaaaaacad4000 add x8, x8, #0x7b0 str x8, [x27] adrp x8, 0xaaaaab361000 <zend_observer_fcall_install+496> // zend_observer_fcall_end add x8, x8, #0x844 blr x8 As one can see, with the tracing JIT, it just forcibly overrides the second arg, inlining the called function. (Which is also why we leaked our custom $context array in the LogsIntegration). The JIT has no exit points here. Hence, we have no choice but to forcefully disallow inlining the hooked methods where overrideArguments is used. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
This PR adds TextMap propagator.
Ping @palazzem