Move collecting and flushing telemetry to after final request flush

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
DataDog · Mar 6, 2024 · bc54d0e · bc54d0e
1 parent 2e1ed12
commit bc54d0e
Show file tree

Hide file tree

Showing 6 changed files with 130 additions and 29 deletions.
diff --git a/components-rs/ddtrace.h b/components-rs/ddtrace.h
@@ -171,6 +171,8 @@ ddog_MaybeError ddog_sidecar_telemetry_buffer_flush(ddog_SidecarTransport **tran
                                                     const ddog_QueueId *queue_id,
                                                     struct ddog_SidecarActionsBuffer *buffer);
 
+void ddog_sidecar_telemetry_buffer_drop(struct ddog_SidecarActionsBuffer*);
+
 ddog_MaybeError ddog_sidecar_connect_php(ddog_SidecarTransport **connection,
                                          const char *error_path,
                                          ddog_CharSlice log_level,

diff --git a/components-rs/telemetry.rs b/components-rs/telemetry.rs
@@ -127,3 +127,7 @@ pub extern "C" fn ddog_sidecar_telemetry_buffer_flush(
 
     MaybeError::None
 }
+
+#[no_mangle]
+pub extern "C" fn ddog_sidecar_telemetry_buffer_drop(_: Box<SidecarActionsBuffer>) {
+}
diff --git a/ext/ddtrace.c b/ext/ddtrace.c
@@ -1017,6 +1017,8 @@ static void dd_disable_if_incompatible_sapi_detected(void) {
     }
 }
 
+static void dd_post_sapi_deactivate_minit(void);
+
 static PHP_MINIT_FUNCTION(ddtrace) {
     UNUSED(type);
 
@@ -1042,6 +1044,7 @@ static PHP_MINIT_FUNCTION(ddtrace) {
 #if ZAI_JIT_BLACKLIST_ACTIVE
     zai_jit_minit();
 #endif
+    dd_post_sapi_deactivate_minit();
 #if PHP_VERSION_ID >= 80100
     ddtrace_setup_fiber_observers();
 #endif
@@ -1374,13 +1377,6 @@ void dd_force_shutdown_tracing(void) {
     DDTRACE_G(in_shutdown) = false;
 }
 
-static void dd_finalize_telemetry(void) {
-    if (DDTRACE_G(telemetry_queue_id)) {
-        ddtrace_telemetry_finalize();
-        DDTRACE_G(telemetry_queue_id) = 0;
-    }
-}
-
 static PHP_RSHUTDOWN_FUNCTION(ddtrace) {
     UNUSED(module_number, type);
 
@@ -1401,7 +1397,19 @@ static PHP_RSHUTDOWN_FUNCTION(ddtrace) {
         DDTRACE_G(active_stack) = NULL;
     }
 
-    dd_finalize_telemetry();
+    if (DDTRACE_G(telemetry_queue_id)) {
+        ddtrace_telemetry_collect_config();
+    }
+
+    return SUCCESS;
+}
+
+static void dd_post_sapi_deactivate_do(void) {
+    if (DDTRACE_G(telemetry_queue_id)) {
+        ddtrace_telemetry_finalize();
+        DDTRACE_G(telemetry_queue_id) = 0;
+    }
+
     if (DDTRACE_G(last_flushed_root_service_name)) {
         zend_string_release(DDTRACE_G(last_flushed_root_service_name));
         DDTRACE_G(last_flushed_root_service_name) = NULL;
@@ -1411,23 +1419,86 @@ static PHP_RSHUTDOWN_FUNCTION(ddtrace) {
         DDTRACE_G(last_flushed_root_env_name) = NULL;
     }
 
-    return SUCCESS;
+    zai_interceptor_deactivate();
+
+    // we can only actually free our hooks hashtables in post_deactivate or later, as within RSHUTDOWN some user code may still run
+    zai_hook_rshutdown();
+    zai_uhook_rshutdown();
+
+    // zai config may be accessed indirectly via other modules RSHUTDOWN, so delay this until the last possible time
+    zai_config_rshutdown();
 }
 
 #if PHP_VERSION_ID < 80000
 int ddtrace_post_deactivate(void) {
 #else
 zend_result ddtrace_post_deactivate(void) {
 #endif
-    zai_interceptor_deactivate();
+    if (sapi_module.deactivate == NULL) { // e.g. in preloading
+        dd_post_sapi_deactivate_do();
+    }
+    return SUCCESS;
+}
 
-    // we can only actually free our hooks hashtables in post_deactivate, as within RSHUTDOWN some user code may still run
-    zai_hook_rshutdown();
-    zai_uhook_rshutdown();
+static int (*dd_prev_sapi_deactivate)(void);
 
-    // zai config may be accessed indirectly via other modules RSHUTDOWN, so delay this until the last possible time
-    zai_config_rshutdown();
-    return SUCCESS;
+// Start of request_rec from httpd.h, this has been unchanged forever, so we can just use this.
+struct apache_request_rec {
+    void *pool;
+    void *connection;
+    void *server;
+    void *next;
+    void *prev;
+    void *main;
+};
+static void (*dd_ap_finalize_request_protocol)(struct apache_request_rec *r);
+
+static int dd_post_apache_deactivate(void) {
+    int result = dd_prev_sapi_deactivate ? dd_prev_sapi_deactivate() : SUCCESS;
+
+    if (SG(sapi_started)) {
+        struct apache_request_rec *r = ((struct {
+            int state;
+            struct apache_request_rec *r;
+        } *) SG(server_context))->r;
+        // Check whether this is not an apache sub-request, otherwise we may not alter the state of the connection, i.e. here we don't gain any latency benefit
+        if (!r->main) {
+            dd_ap_finalize_request_protocol(r);
+        }
+
+        dd_post_sapi_deactivate_do();
+    }
+
+    return result;
+}
+
+static int dd_post_sapi_deactivate(void) {
+    int result = dd_prev_sapi_deactivate ? dd_prev_sapi_deactivate() : SUCCESS;
+    if (SG(sapi_started)) { // only if a proper request happened; it's also called during minit
+        dd_post_sapi_deactivate_do();
+    }
+    return result;
+}
+
+static void dd_post_sapi_deactivate_minit(void) {
+    dd_prev_sapi_deactivate = sapi_module.deactivate;
+
+    if (!ddtrace_disable && ddtrace_active_sapi == DATADOG_PHP_SAPI_APACHE2HANDLER) {
+#ifndef _WIN32
+        void *handle = dlopen(NULL, RTLD_LAZY);
+#else
+        void *handle = GetModuleHandle(NULL); // not refcounted, must not be closed
+#endif
+        dd_ap_finalize_request_protocol = DL_FETCH_SYMBOL(handle, "ap_finalize_request_protocol");
+#ifndef _WIN32
+        dlclose(handle);
+#endif
+        if (dd_ap_finalize_request_protocol) {
+            sapi_module.deactivate = dd_post_apache_deactivate;
+            return;
+        }
+    }
+    sapi_module.deactivate = dd_post_sapi_deactivate;
 }
 
 void ddtrace_disable_tracing_in_current_request(void) {
@@ -2056,7 +2127,11 @@ PHP_FUNCTION(dd_trace_internal_fn) {
     RETVAL_FALSE;
     if (ZSTR_LEN(function_val) > 0) {
         if (FUNCTION_NAME_MATCHES("finalize_telemetry")) {
-            dd_finalize_telemetry();
+            if (DDTRACE_G(telemetry_queue_id)) {
+                ddtrace_telemetry_collect_config();
+                ddtrace_telemetry_finalize();
+                DDTRACE_G(telemetry_queue_id) = 0;
+            }
             RETVAL_TRUE;
         } else if (params_count == 1 && FUNCTION_NAME_MATCHES("detect_composer_installed_json")) {
             ddog_CharSlice path = dd_zend_string_to_CharSlice(Z_STR_P(ZVAL_VARARG_PARAM(params, 0)));

diff --git a/ext/ddtrace.h b/ext/ddtrace.h
@@ -101,6 +101,7 @@ ZEND_BEGIN_MODULE_GLOBALS(ddtrace)
 
     char *cgroup_file;
     ddog_QueueId telemetry_queue_id;
+    ddog_SidecarActionsBuffer *telemetry_buffer;
     ddog_AgentRemoteConfigReader *remote_config_reader;
     HashTable *agent_rate_by_service;
     zend_string *last_flushed_root_service_name;

diff --git a/ext/telemetry.c b/ext/telemetry.c
@@ -25,23 +25,16 @@ void ddtrace_telemetry_first_init(void) {
     dd_composer_hook_id = zai_hook_install((zai_str)ZAI_STR_EMPTY, (zai_str)ZAI_STR_EMPTY, dd_check_for_composer_autoloader, NULL, ZAI_HOOK_AUX_UNUSED, 0);
 }
 
-void ddtrace_telemetry_finalize(void) {
+void ddtrace_telemetry_collect_config(void) {
     if (!ddtrace_sidecar || !get_global_DD_INSTRUMENTATION_TELEMETRY_ENABLED()) {
         return;
     }
 
-    ddog_SidecarActionsBuffer *buffer = ddog_sidecar_telemetry_buffer_alloc();
+    if (DDTRACE_G(telemetry_buffer)) {
+        ddog_sidecar_telemetry_buffer_drop(DDTRACE_G(telemetry_buffer));
+    }
 
-    zend_module_entry *module;
-    char module_name[261] = { 'e', 'x', 't', '-' };
-    ZEND_HASH_FOREACH_PTR(&module_registry, module) {
-        size_t namelen = strlen(module->name);
-        memcpy(module_name + 4, module->name, MIN(256, strlen(module->name)));
-        const char *version = module->version ? module->version : "";
-        ddog_sidecar_telemetry_addDependency_buffer(buffer,
-                                                    (ddog_CharSlice) {.len = namelen + 4, .ptr = module_name},
-                                                    (ddog_CharSlice) {.len = strlen(version), .ptr = version});
-    } ZEND_HASH_FOREACH_END();
+    ddog_SidecarActionsBuffer *buffer = DDTRACE_G(telemetry_buffer) = ddog_sidecar_telemetry_buffer_alloc();
 
     for (uint8_t i = 0; i < zai_config_memoized_entries_count; i++) {
         zai_config_memoized_entry *cfg = &zai_config_memoized_entries[i];
@@ -69,6 +62,31 @@ void ddtrace_telemetry_finalize(void) {
             ddog_sidecar_telemetry_addIntegration_buffer(buffer, integration_name, (ddog_CharSlice)DDOG_CHARSLICE_C(""), false);
         }
     }
+}
+
+void ddtrace_telemetry_finalize(void) {
+    if (!ddtrace_sidecar || !get_global_DD_INSTRUMENTATION_TELEMETRY_ENABLED()) {
+        return;
+    }
+
+    ddog_SidecarActionsBuffer *buffer = DDTRACE_G(telemetry_buffer);
+    if (buffer) {
+        DDTRACE_G(telemetry_buffer) = NULL;
+    } else {
+        buffer = ddog_sidecar_telemetry_buffer_alloc();
+    }
+
+    zend_module_entry *module;
+    char module_name[261] = { 'e', 'x', 't', '-' };
+    ZEND_HASH_FOREACH_PTR(&module_registry, module) {
+        size_t namelen = strlen(module->name);
+        memcpy(module_name + 4, module->name, MIN(256, strlen(module->name)));
+        const char *version = module->version ? module->version : "";
+        ddog_sidecar_telemetry_addDependency_buffer(buffer,
+                                                    (ddog_CharSlice) {.len = namelen + 4, .ptr = module_name},
+                                                    (ddog_CharSlice) {.len = strlen(version), .ptr = version});
+    } ZEND_HASH_FOREACH_END();
+
     ddog_sidecar_telemetry_buffer_flush(&ddtrace_sidecar, ddtrace_sidecar_instance_id, &DDTRACE_G(telemetry_queue_id), buffer);
 
     ddog_CharSlice service_name = DDOG_CHARSLICE_C("unnamed-php-service");

diff --git a/ext/telemetry.h b/ext/telemetry.h
@@ -6,6 +6,7 @@
 void ddtrace_telemetry_first_init(void);
 ddog_TelemetryWorkerHandle *ddtrace_build_telemetry_handle(void);
 void ddtrace_telemetry_notify_integration(const char *name, size_t name_len);
+void ddtrace_telemetry_collect_config(void);
 void ddtrace_telemetry_finalize(void);
 
 #endif // DDTRACE_TELEMETRY_H