Moving spans from userland to extension by bwoebi · Pull Request #1251 · DataDog/dd-trace-php

bwoebi · 2021-06-04T16:24:04Z

Description

Doing a lot of things:

Nearly everything happening in Tracer->flush() is now in the extension
Spans are allocated and closed directly instead of their ids
Spans now expose duration and start time via methods
The root span is (if there is configured to be one) allocated and closed from within the extension - close at the very last moment: RSHUTDOWN

Also note: DRAFT, UNTESTED AND BROKEN.

Readiness checklist

(only for Members) Changelog has been added to the release document.
Tests added for this feature/bug.

Reviewer checklist

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

labbati

Thank you Bob, added a few comments in the meantime. I am also going to test a specific edge case I have in mind and that I think is not covered in a test, but I will report back on that with an additional comment

labbati · 2021-06-07T08:59:05Z


        if (\dd_trace_env_config('DD_TRACE_GENERATE_ROOT_SPAN')) {
-            self::initRootSpan($tracer);
+            self::  initRootSpan($tracer);


whitespaces

labbati · 2021-06-07T09:12:28Z

+
+            if (PHP_VERSION_ID >= 80000) {
+                foreach ($httpHeaders as $header => $value) {
+                    if (stripos($header, Propagator::DEFAULT_ORIGIN_HEADER) === 0) {


This can fail in case one day any datadog product calling into PHP - e.g. DD Synthetics - adds a tag with the same prefix, e.g.x-datadog-origin-format (that I am totally making up).

I would prefer something like if (strtolower($header) === Propagator::DEFAULT_ORIGIN_HEADER) {...}.

In case you are concerned about the performance impact of lowering potentially many headers, we can lower only when we have to, meaning ...

if (stripos($header, Propagator::DEFAULT_ORIGIN_HEADER) === 0 && strtolower($header) === Propagator::DEFAULT_ORIGIN_HEADER).

I'd suggest to add a test for similar cases as well, and naming the additional header x-datadog-origin-format can be the perfect example I intend the test for.

while that's totally possible, it's in line with the current handling: https://github.com/DataDog/dd-trace-php/blob/master/src/DDTrace/Propagators/CurlHeadersMap.php#L33 If we want to change that, we probably should do that separately...?

good point, please create a ticket that we can address later as a low hanging fruit

labbati · 2021-06-07T09:39:29Z

        }

-        return $this->scopeManager->activate($span, $options->shouldFinishSpanOnClose());
+        return $this->scopeManager->activate($span, $options->shouldFinishSpanOnClose() && (PHP_VERSION_ID >= 80000 && $span->getParentId() == 0));


if this logic is what you meant to add, then parenthesis (, ) are redundant.
I am not getting this logic though (I am still going through the remaining code). Is it possible that you meant && (PHP_VERSION_ID < 80000 || $span->getParentId() == 0)?

In case this holds true, then we are definitely missing a test here :)

Yeah that's not right. And there's also a failing test for that. (but as said, this current PR is fully untested, locally I've done some progress.)

labbati · 2021-06-07T10:00:05Z

+
+    zend_string *key, *val;
+    if (zend_parse_parameters(ZEND_NUM_ARGS(), "SS", &key, &val) == FAILURE) {
+        RETURN_THROWS();


We should not throw. We can log a message in debug mode and proceed the execution.
In general, we never throw exceptions, so users' applications can keep working even if with tracing not perfectly working

labbati · 2021-06-07T10:01:40Z

+    // TODO: also prevent closing internal root spans? Would need marker though
+    if (!DDTRACE_G(open_spans_top) || !DDTRACE_G(open_spans_top)->execute_data) {
+        zend_throw_exception(NULL, "There is no user-span on the top of the stack. Cannot close.", 0);
+        RETURN_THROWS();


Same as above, let's not throw. The maximum that we can accept (but we should avoid it) is that tracing is not working as expected, but users' requests never have to be interrupted because of us.

labbati · 2021-06-07T10:07:48Z

+    zend_string *tag_key;
+    zval *tag_value;
+    ZEND_HASH_FOREACH_STR_KEY_VAL(DDTRACE_G(additional_global_tags), tag_key, tag_value) {
+        zend_hash_add(DDTRACE_G(additional_global_tags), tag_key, tag_value);


Not sure I understand what this line does. I think that here we should add from DDTRACE_G(additional_global_tags) into Z_ARR_P(meta)?

If this holds true, then we are missing a test for global + additional globals?

Yes - and yes, for sure that'll get a test.

labbati · 2021-06-07T10:27:42Z

+                        // this also gets set when creating a root span, but may not have the latest up-to-date data
+                        if ('cli' !== PHP_SAPI && \ddtrace_config_url_resource_name_enabled()
+                            && $rootScope = $this->getRootScope()) {
+                            $this->addUrlAsResourceNameToSpan($rootScope->getSpan());
+                        }
+                        /*
+                         * Having this priority sampling here is actually a bug (should happen after service name
+                         * substitutions), but it was this way before the refactor, so let's fix this in a
+                         * subsequent release, when we will have ported everything else to the extension.
+                         */
+                        $tracer = GlobalTracer::get();
+                        if (method_exists($tracer, "enforcePrioritySamplingOnRootSpan")) {
+                            $tracer->enforcePrioritySamplingOnRootSpan();
+                        }


I am not understanding this part. Reaching out in private to have a quick sync.

SammyK

Looking great so far! I'm still reviewing this but I left a few comments/questions so far.

SammyK · 2021-06-08T14:04:39Z

+const zend_function_entry class_DDTrace_SpanData_methods[] = {
+    PHP_ME(DDTrace_SpanData, getDuration, arginfo_ddtrace_void, ZEND_ACC_PUBLIC)
+        PHP_ME(DDTrace_SpanData, getStartTime, arginfo_ddtrace_void, ZEND_ACC_PUBLIC) PHP_FE_END};


The linter strikes again. :) Wrapping this with // clang-format {off|on} should keep it from getting mangled by the linter.

SammyK · 2021-06-08T14:30:04Z

+/* {{{ proto string dd_trace_peek_span() */
+static PHP_FUNCTION(dd_trace_peek_span) {
+    UNUSED(execute_data);
+    if (DDTRACE_G(open_spans_top)) {
+        RETURN_OBJ_COPY(&DDTRACE_G(open_spans_top)->span.std);
+    }
+    RETURN_NULL();
+}


I assume this API is named similar to dd_trace_peek_span_id(), but since we're namespacing all new functions under DDTrace\, that function will likely become DDTrace\active_span_id() (to match the common "active span" terminology). Or we might remove it entirely since the span ID can be obtained from SpanData. WDYT of naming this function DDTrace\active_span()?

The span id is is currently not exposed on SpanData, but we may add getters for that...
DDTrace\active_span() sounds good to me.

Required to attach errors/exceptions to spans after userland is fully destroyed (e.g. in a bailout in shutdown) ... and unblocks incremental migration Signed-off-by: Bob Weinand <[email protected]>

labbati reviewed Jun 7, 2021

View reviewed changes

bwoebi force-pushed the bob/internal-rootspan branch from cbb32e9 to c954625 Compare June 8, 2021 13:16

SammyK reviewed Jun 8, 2021

View reviewed changes

bwoebi mentioned this pull request Jun 8, 2021

Move mapping-settings to be deserialized as a zend_array * #1253

Merged

5 tasks

bwoebi force-pushed the bob/internal-rootspan branch from 00b89fb to db9ff73 Compare June 9, 2021 09:12

bwoebi mentioned this pull request Jun 9, 2021

Refactor internal span objects to be allocated together with their internal data #1254

Merged

5 tasks

Moving spans from userland to extension

4af6430

Required to attach errors/exceptions to spans after userland is fully destroyed (e.g. in a bailout in shutdown) ... and unblocks incremental migration Signed-off-by: Bob Weinand <[email protected]>

bwoebi force-pushed the bob/internal-rootspan branch from db9ff73 to 4af6430 Compare June 9, 2021 10:20

bwoebi closed this Jun 9, 2021

bwoebi deleted the bob/internal-rootspan branch June 9, 2021 10:26

bwoebi restored the bob/internal-rootspan branch June 9, 2021 10:26

bwoebi mentioned this pull request Jun 21, 2021

Moving spans from userland to extension #1262

Merged

5 tasks

Conversation

bwoebi commented Jun 4, 2021

Description

Readiness checklist

Reviewer checklist

Uh oh!

labbati left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SammyK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bwoebi Jun 8, 2021 •

edited

Loading